[processor/groupbyattrs] Updated metric copy to take into consideration custom properties for … #9088
Conversation
|
|
crobertson-conga
force-pushed
the
copy_agg_type_9087
branch
from
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 |
codeboten
changed the title
…Histogram, ExponentialHistogram and Sum type metrics.
crobertson-conga
force-pushed
the
copy_agg_type_9087
branch
from
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.
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?
|
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 |
crobertson-conga
force-pushed
the
copy_agg_type_9087
branch
from
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) |
dmitryax
added
the
ready to merge
|
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