Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add New Trace Logging Level #749

Closed
wants to merge 4 commits into from
Closed

Conversation

LeviMatus
Copy link

A few people have expressed interest in having a TraceLevel added to Zap (Some mention of this in #680 and again in #747). This PR addresses that interest by introducing this logging level.

Added the ability to specify logging at a lower TraceLevel.
Changes to zapcore/level.go, zapcore/level_strings.go, and level.go
were made.

- zapcore/level.go saw TraceLevel introduced as iota-2, thus preserving
the previous value of level constants. The _minLevel was also set to be
TraceLevel. Unmarshal methods now have trace strings included.

- zapcore/level_strings saw a new level color added. Trace level is
now associated with the color Green.

- Adds TraceLevel tests to level table
  driven tests. Added appropriate level
  strings to TestLevelText, TestLevelString,
  TextCapitalLevelsParse, and TestWeirdLevelsParse.
- level.go saw TraceLevel introduced, referencing the
new zapcore TraceLevel

- Adds TraceLevel tests to level table
  driven tests. Added appropriate table
  entries to TestLevelEnablerFunc and
  TestAtomicLevelText
- Add Trace logging method to logger.go

- Adds AtomicLevel table test entries for
 TraceLevel usage.
@LeviMatus LeviMatus changed the title Trace level Add New Trace Logging Level Oct 24, 2019
@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

Merging #749 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #749      +/-   ##
==========================================
+ Coverage   97.44%   97.46%   +0.01%     
==========================================
  Files          40       40              
  Lines        2117     2126       +9     
==========================================
+ Hits         2063     2072       +9     
  Misses         46       46              
  Partials        8        8              
Impacted Files Coverage Δ
level.go 100.00% <ø> (ø)
zapcore/level_strings.go 100.00% <ø> (ø)
logger.go 94.64% <100.00%> (+0.14%) ⬆️
zapcore/level.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0065243...4e1209c. Read the comment docs.

Adds TraceLevel test case to LeveledMethods
table test. Changes the base logger to be the
TraceLevel, ensuring Trace and all higher
levels get tested against.
@LeviMatus LeviMatus closed this Oct 24, 2019
@LeviMatus LeviMatus reopened this Oct 24, 2019
@CLAassistant
Copy link

CLAassistant commented Oct 24, 2019

CLA assistant check
All committers have signed the CLA.

@prashantv
Copy link
Collaborator

I'm not sure that we can add a level to zap 1.0 and retain backwards compatibility. Existing cores, hooks, etc do not know about the trace level and may not handle it properly. For example, here's an adapter library that checks for known levels:

https://github.com/uber-common/bark/blob/master/zbark/zapify.go#L64

That library at least handles it will by returning an error, but there may be other cases where it panics.

I don't think we can add any new levels unless we do a zap 2.0.

@LeviMatus
Copy link
Author

LeviMatus commented Oct 25, 2019

Thanks for the code reference there to check against. Having external wrappers is something I had not thought of and is a great point. I see now how this could make introducing new levels more difficult.

Moreover, I just realized that I neglected to add Trace methods to sugar.go, so the PR can't even be considered complete at this time.

@novabyte
Copy link

novabyte commented Dec 9, 2019

@LeviMatus You've done great work on this pull request! It's sorely needed within Zap even if we cannot specify our own set of severity levels for output.

@prashantv Is there any kind of timeline for when a set of changes like this would be acceptible to the release constraints of Zap?

@prashantv
Copy link
Collaborator

prashantv commented Dec 9, 2019

@prashantv Is there any kind of timeline for when a set of changes like this would be acceptible to the release constraints of Zap?

We don't currently have a concrete timeline for zap 2.0, but the improvements we'd like to make in a 2.0 are tracked here: #388.

@se3000
Copy link

se3000 commented Mar 23, 2020

Any update on this? We would like to use it.

@Antonboom
Copy link

What's the bottom line? Wait for zap v2?

@prashantv
Copy link
Collaborator

We definitely can't add it in 1.0. It's possible for 2.0, but that's not in the roadmap. At that point, we'd want to consider the cost/benefit (it's added cognitive overhead to add another level, so we should ensure there's a significant benefit).

Closing for 1.0.

@prashantv prashantv closed this Nov 18, 2020
@Andrea-Campanella
Copy link

@prashantv I would say that there is significant benefit to introduce Trace level due to otherwise overwhelming debug logs. we have found ourselves in a position where DEBUG has way too many messages because we can't demote some to a lower (non existing) level but we can't also promote anything to INFO level due to the nature of the log. thoughts ?

@abhinav abhinav mentioned this pull request Feb 18, 2021
@prashantv prashantv mentioned this pull request Apr 12, 2021
This was referenced Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants