-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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/groupbyattrs] Updated metric copy to take into consideration custom properties for … #9088
[processor/groupbyattrs] Updated metric copy to take into consideration custom properties for … #9088
Conversation
5df881f
to
14aa6f6
Compare
Thank you for the contribution @crobertson-conga Before we can proceed you need to sign the CLA first. Contributing guide has the details on that |
…Histogram, ExponentialHistogram and Sum type metrics.
14aa6f6
to
5d410ea
Compare
Took me a bit to get the CLA approved, but that is now in place. |
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.
Added one more comment on logging but I think it's not a blocker.
Also seems something is off with recent pdata change. Linter and unittest fail. For the latter the error is:
FAIL github.com/open-telemetry/opentelemetry-collector-contrib/processor/groupbyattrsprocessor [setup failed]
Error: processor_test.go:26:2: missing go.sum entry for module providing package go.opentelemetry.io/collector/model/pdata (imported by github.com/open-telemetry/opentelemetry-collector-contrib/processor/groupbyattrsprocessor); to add:
Second pair of eyes would be helpful :) Perhaps @jpkrohling could you look as well?
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.
Metrics aren't my strongest side, not sure I can really judge the business here.
Is there anything else that needs to be done on this in order to get this in the next release? |
I think @jpkrohling comment needs to be addressed. I think that copy/pasting (or even better adding it to some common package) the sampled logger is the simplest solution |
e1c18b1
to
3a47f20
Compare
Okay, I think I've done all the needed things for this now. |
Is there anything else that needs to be done here? |
We could use another pair of eyes :) Not sure if @jpkrohling will manage to take a look today, if not, maybe one of the other @open-telemetry/collector-contrib-approvers could help? |
I'll add it to my queue, but I can't promise I'll be able to review this at all. |
Fixed the changelog conflict, can we get this merged? |
Any chance of getting this in soon? |
@crobertson-conga can you please address the latest comments in #9088 (comment) |
Just resolved the changelog conflicts again, can we get this merged? |
This is now labeled as such, which means that one of the maintainers will probably handle that very soon @crobertson-conga |
@codeboten pleases help to merge this |
…on custom properties for … (open-telemetry#9088) * Updated metric copy to take into consideration custom properties for Histogram, ExponentialHistogram and Sum type metrics. * Added error message for unknown data types. * Fixed parameter count and actually ran make. * Addressed data object renaming and lint errors. * Add in sampled logging. * Updated changelog with fix * Added monotonic property. * Removed unnecessary missing metric logs. * Reverted factory changes. Co-authored-by: Alex Boten <aboten@lightstep.com>
…Histogram, ExponentialHistogram and Sum type metrics.
Description:
Fixing a bug by copying over source temporality change
Link to tracking Issue: 9087
Testing:
Updated tests for the processor to have a histogram metric type.
Documentation: \
Fixes: #9981