[receiver/prometheus] Generate Scope Attributes from otel_scope_<attribute-name> labels#46612
Conversation
3c55be3 to
f7f7e9c
Compare
67df249 to
aa64109
Compare
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: | | ||
| Scope attributes are always extracted from `otel_scope_<attribute-name>` labels on metrics. |
There was a problem hiding this comment.
Isn't it also true now that we drop any extra scope attributes from the resulting attributes? Which is a breaking change if somebody relied on them?
There was a problem hiding this comment.
I'm not sure what you mean here. Could you elaborate?
There was a problem hiding this comment.
Add something like "labels starting with otel_scope_" are no longer included as metric attributes, and are put in scope attributes instead"
| name string | ||
| version string | ||
| schemaURL string | ||
| attrsHash uint64 |
There was a problem hiding this comment.
I'm not sure about the cardinality of scopes , but you can run into conflicts with hashes.
Since we sort attributes (I think), maybe it would be better to just make a concatenated copy of the scope attributes here using some non utf-8 delimeter ?
There was a problem hiding this comment.
Yeah, I tried that, but it has a big performance penalty. Building the string adds 10-15% to allocs and seconds/op for the whole Append method.
I don't understand hash collision well enough to say this penalty is worth it for better reliability, though 😅
There was a problem hiding this comment.
I think we use hashes all over the place in the receiver. From my testing with the otel-go SDK, collisions aren't remotely likely until you get into the ~10 billion cardinality range for the cache.
If we want to revisit this, lets do it outside of this PR.
| base := getSortedNotUsefulLabels(mType) | ||
| exclusions := make([]string, 0, len(base)+ls.Len()) | ||
| exclusions = append(exclusions, base...) | ||
| seen := make(map[string]struct{}, len(base)) |
There was a problem hiding this comment.
How frequently do we call this function? Isn't this an extra allocation for every sample? Could we add a benchmark ?
There was a problem hiding this comment.
Yes, good catch. I'm adding a commit so we only allocate exclusions if we find a label prefixed with otel_scope_, otherwise we just return base.
There was a problem hiding this comment.
Regarding the benchmark, we already have a testbed. I can run it if you feel like this really needs to be properly benchmarked :)
aa64109 to
7b6c049
Compare
|
I went through your feedback @krajorama, but couldn't find the time to address everything. I'll let you know once all comments have been addressed! |
a9da81f to
c3b016e
Compare
|
This should be ready for another round of reviews @krajorama @dashpole :) |
2b0dc35 to
cafa4ff
Compare
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
cafa4ff to
2b45ea6
Compare
…etry#5004) Fixes open-telemetry#4752 ## Changes Stabilizes the conversion of labels prefixed with `otel_scope_` to OTLP Instrumentation scope. Implementation has been done in open-telemetry/opentelemetry-collector-contrib#46612 For non-trivial changes, follow the [change proposal process](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CONTRIBUTING.md#proposing-a-change). * [X] Related issues open-telemetry#4752 * [ ] Related [OTEP(s)](https://github.com/open-telemetry/oteps) # * [ ] Links to the prototypes (when adding or changing features) * [X] [`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md) file updated for non-trivial changes * For trivial changes, include `[chore]` in the PR title to skip the changelog check * [ ] [Spec compliance matrix](https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix/template.yaml) updated if necessary --------- Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com> Co-authored-by: David Ashpole <dashpole@google.com>
Description
Add support for extracting scope attributes from otel_scope_ labels on metrics in the Prometheus receiver, per the spec change.
This is gated behind
receiver.prometheusreceiver.UseScopeAttributeLabels(alpha, disabled by default). The gate acts as a switch:Link to tracking issue
Fixes #41502
Testing