-
Notifications
You must be signed in to change notification settings - Fork 160
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
Update attribute processor for stable metrics #524
Conversation
a949fa0
to
5acd512
Compare
I would like to understand the use case for this change, whyy the need to inherit AttributeProcessor instead of extending it |
There're common labels for all metrics in our iOS app. Attribute processor can be used to merge them into final labels. But one of these labels can be changed by user or server. Customized attribute processor can be more flexible to support it. Something like this:
Alternatively, it can return a |
Previously AttribtueProcessor is a class and not open. It can't be customized externally. Now it's changed to procotol and its static functions are moved into SimpleAttributeProcessor (now it's public). Also duplicated NoopAttributesProcessor is removed and more tests are added.
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.
LGTM, I had some doubts at the beginning but keeping the base code as part of the protocol with an extension makes total sense. Thanks a lot for your contribution,
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
=======================================
Coverage ? 67.45%
=======================================
Files ? 335
Lines ? 14585
Branches ? 0
=======================================
Hits ? 9839
Misses ? 4746
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Previously AttribtueProcessor is a class and not open. It can't be customized externally. Now it's changed to procotol and its static functions are moved into SimpleAttributeProcessor (now it's public).
Also duplicated NoopAttributesProcessor is removed and more tests are added.