Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
core/sampler: Support decision hook #813
core/sampler: Support decision hook #813
Changes from 13 commits
14b1350
c59b269
e268359
cefe15e
1f5cf2c
44fe037
25478e6
5cc987e
fd20684
4ae250b
d48791a
3c00734
0bc5a78
8b91fbb
09045e2
6d960ec
ae3e230
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we fix the off-by-one, seems a little odd, haven't looked into where it's coming from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I didn't verify the comment and copied from another test. Fixed explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this the same as
expectN
? we expect the hook to be called on every log, and thenexpectDropped + expectSampled == expectN
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expectN
is not a count from samplerHook, but from a zap hook which only measures what is sent. i regret carrying over a hook from another test now as it seems to cause confusion. :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should comment that more decision values may be added in future, so it's not safe to assume these are the only values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this is so that it takes one byte instead of 8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, as byte is an alias to uint8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something to discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an idea: If we make SamplingDecision a bitfield, we could do that separately and re-use sampler logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea, I like the idea of a bit field, so users can just check
decision & LogSampled > 0
to see if it was sampled.we might want to make that clear in the comments that users should check this like a bit field (or maybe we provide a helper on the type to do that).
We could expand this to have:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need to export this? We'll use this if unset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if someone wants to test this as NewSampler* constructors are public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewSampler can be public but I mean the implementation of a no-op hook. Users don't really need access to that; they can write their own empty function, or just not provide the SamplingHook option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, not even sure if we need this in the production code, we could keep it in the test file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's production. The sampler defaults the hook field to NopSamplingHook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, in that case, can we unexport and keep it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding a sample of how to use the sampling decision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to reduce the added API surface (E.g., new function that looks similar + new types), we could add a
Build
method on theSamplingConfig
struct, similar to theBuild
method on theConfiguration
struct:https://pkg.go.dev/go.uber.org/zap?tab=doc#Config.Build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we still want a means of building the sampler directly?
Config.Build is porcelain for zap.New. Anything you can do with Config.Build,
you can do with zap.New.
IMO SamplingConfig would need to match that: the underlying constructor would
need to provide the same functionality—unless we decide to deprecate direct
constructors and switch to Config structs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, I'm not sure how valuable the direct structs have been vs configs, but don't have to make that change here. let's keep this as-is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: count dropped messages only to more accurately measure the use case we're trying to satisfy here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is
cc
and why can't this be updated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code coverage