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

Expose additional filters for observed logs #943

Merged
merged 3 commits into from
Jun 8, 2021

Conversation

jkanywhere
Copy link
Contributor

There are various ways I examine logs when testing how my library uses zap.
Filtering by log level seems generically useful.
Other filters may not be re-usable enough to include directly in zap, so expose the ability to
filter by an arbitrary function.

@codecov
Copy link

codecov bot commented May 15, 2021

Codecov Report

Merging #943 (4223fa6) into master (3748251) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #943   +/-   ##
=======================================
  Coverage   98.02%   98.02%           
=======================================
  Files          44       44           
  Lines        1974     1978    +4     
=======================================
+ Hits         1935     1939    +4     
  Misses         30       30           
  Partials        9        9           
Impacted Files Coverage Δ
zaptest/observer/observer.go 100.00% <100.00%> (ø)
zapcore/field.go 100.00% <0.00%> (ø)
clock.go
zapcore/clock.go 100.00% <0.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 3748251...4223fa6. Read the comment docs.

Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FilterLevel is an obvious yes.

Arbitrary Filter, I'm not opposed to exposing. @prashantv?

zaptest/observer/observer.go Outdated Show resolved Hide resolved
zaptest/observer/observer.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FilterLevel is an obvious yes.

Arbitrary Filter, I'm not opposed to exposing. @prashantv?

I think it's useful, +1 for allowing arbitrary filter.

zaptest/observer/observer.go Outdated Show resolved Hide resolved
Export method  `observer.(*ObservedLogs).Filter` to allow consumers to
filter logs by arbitrary functions for testing.

Currently consumers can call `(*ObservedLogs).All` and proceed to filter
using `[]LoggedEntry` directly, but the result is then incompatible with
existing methods such as `FilterMessage` because there is now way to
re-construct an `*ObservedLogs` object.
Method `observer.(*ObservedLogs).FilterLevelExact` allows consumers to select
entries logged at a given level, for testing.
@jkanywhere jkanywhere force-pushed the expose-filter-for-observed-logs branch from 2fa9151 to 130ef27 Compare May 23, 2021 21:16
Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ nit

zaptest/observer/observer_test.go Outdated Show resolved Hide resolved
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.

3 participants