-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[processor/logdedup] Add include_fields option #37219
Conversation
ebd4402
to
1731356
Compare
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.
Overall I like the idea but think it would be better to use a set of keys instead of just one.
6a2a9d9
to
fb7810c
Compare
fb7810c
to
e243145
Compare
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.
Look good, thanks for your work on this @mauri870. I've left some minor feedback but think it's close 👍🏻
Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
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.
Looks good for me. Any feedback from you @djaglowski?
#### Link to tracking issue Fixes open-telemetry#36965 <!--Describe what testing was performed and which tests were added.--> #### Testing Added tests that validate the new `include_fields` option as well as integration tests with ConsumeLogs. <!--Describe the documentation added.--> #### Documentation Added documentation for the new `include_fields` option as well as a README example. --------- Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
Link to tracking issue
Fixes #36965
Testing
Added tests that validate the new
include_fields
option as well as integration tests with ConsumeLogs.Documentation
Added documentation for the new
include_fields
option as well as a README example.