aggregateutil: allow filtering against empty attribute set #35006
Conversation
It is a valid use case to aggregate against an empty label set, which will functionally clear all attributes. This behaviour was removed in a previous PR, which simplified the check to `len() == 0`, which covers the case of the label set being `nil` and having 0 elements the same. However, these are not the same scenario and have different meanings. This PR reintroduces the original behaviour, but in a more efficient way by recognizing a label set with 0 elements and clearing the attributes, which would be the logical conclusion after running the filter anyway.
|
CC @odubajDT |
|
Integration test looks like a flake to me. I don't have authority to re-run the CI with the button but I can force-push if I need to re-run. |
|
please run |
|
Is there any chance this PR can be reviewed? |
There was a problem hiding this comment.
@braydonk to make sure the functionality is the same as before can you also restore any tests to the metricstransformprocessor that covers this scenario.
Adds empty and nil label set tests. Exposed something I didn't notice in the way the code is called when calling from metricstransform processor compared to using the aggregateutil package functions directly.
github-actions
bot
added
the
processor/metricstransform
|
This is done in f7c46ce. Thanks for calling it out, I should have thought of that and it did expose a problem that I fixed in that commit as well. While manually testing this along with the unit tests, I found something interesting. While in the previous version of the In f7c46ce I adjusted it to check for |
Ya lets restore the previous behavior, it doesn't sound like the way metricstransform was doing this before was a bug. |
Even before the aggregateutil change, metricstransform never passed a nil label_set to `aggregateLabelsOp`; a nil label_set would always be changed to empty slice anyway. This commit restores that behaviour.
The length of the attributes array can always be a known value. This commit pre-allocates that space to save on allocations.
|
Fine by me. Having a nil Restored in debfca5 |
|
@TylerHelmuth @braydonk is there anything else holding up this fix? I'm just wondering on the ETA to get this into a release. |
|
It is only held up by review at the moment. I will resolve the conflict when the review is done; it's just a matter of doing |
|
@braydonk The generated code for the contrib distro has been removed from the repo for now, so you can delete the |
processor/metricstransformprocessor/metrics_transform_processor_otlp.go
Outdated
Show resolved
Hide resolved
processor/metricstransformprocessor/metrics_transform_processor_otlp.go
Outdated
Show resolved
Hide resolved
I called `make` in a way that pre-fills the slice rather than just setting the capacity.
Done in 2d6985e |
|
Never seen a failure like in https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/11236881640/job/31238370011?pr=35006 🤔 Maybe just needs a re-run? |
|
@open-telemetry/collector-contrib-maintainers The vulncheck workflows are now passing on this PR. I think this just needs a maintainer approval and would be ready to merge. |
Description:
It is a valid use case to aggregate against an empty label set, which will functionally clear all attributes. This behaviour was removed in #33655, which simplified the check to
len() == 0, which covers the case of the label set beingniland having 0 elements as the same scenario. However, these are not the same scenario and have different meanings. This PR reintroduces the original behaviour, but in a more efficient way by recognizing a label set with 0 elements and clearing the attributes, which would be the logical conclusion after running the filter anyway.Link to tracking Issue: #34430
Testing:
Documentation: