-
Notifications
You must be signed in to change notification settings - Fork 889
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
Proposal for flattening attributes from OTLP messages #2736
Proposal for flattening attributes from OTLP messages #2736
Conversation
### Traces | ||
|
||
``` | ||
Span.attributes > ScopeSpans.scope.attributes > ResourceSpans.resource.attributes |
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.
What is a ScopeSpan?
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.
ScopeSpans is the wrapper combining multiple spans and a Scope: https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/trace/v1/trace.proto#L64
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.
I see. If I were implementing a flattening scheme, I would keep ALL attributes, e.g. by adding distinguishable prefix for each category, instead of letting them override each other.
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.
@yurishkuro yes. Another way of putting this: Why flatten in the first place?
The fact that OpenMetrics specifies a "flat" set of attributes does not mean we should flatten resource and scope-level attributes. OpenMetrics specifies how to join resource attributes using the target_into
metric, and @dashpole has #2703 in progress, specifying how to join scope attributes using an opentelemetry_scope_info metric.
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.
Another way of putting this: Why flatten in the first place?
No, I don't have an issue with flattening, it may be necessary due to the limitations of a target telemetry platform. But it does not mean that flattening should be a lossy transformation, which is what you're proposing.
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.
The big argument against adding a prefix is that it changes the attribute key. Users can not query for the attribute they added, since the name changed. This impacts semantic conventions as well: Should all semantic conventions be prefixed? I feel like we would need to do that to stay consistent.
But it does not mean that flattening should be a lossy transformation, which is what you're proposing.
That is true, but this is the idea behind this proposal: If you want to, you can overwrite attributes. If you don't want to overwrite attributes, you can rename them (e.g., add a prefix explicitly). In either case, you will get the attributes that you defined, and don't have to go looking for the renamed version.
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.
I haven't thought of doing renaming instead of overwriting, but it is worth considering as an option.
A couple thoughts that may help:
- Attribute conflicts are likely going to be very rare. AFAIK, there isn't currently any real semantic convention that uses the same attribute name on different levels.
- There are existing implementations that flatten and overwrite, see e.g. Zipkin exporter in Collector so we have a precedent.
- I do like the fact that prefixing makes flattening a non-destructive operation. However, in real-world use cases you are likely mostly interested in the most specific value of the attribute, recorded at the innermost level, so overwriting seems to be a natural behavior.
If we could have real examples where attribute names can conflict it would help to make a decision. The responsible_team
example I is confusing because we do have a convention for that already and it is called service.namespace
and is supposed to be recorded on the Resource only. Any other examples that we can look at?
If opinions are split on this it is also possible to make this behavior configurable (i.e. to overwrite or to prefix) but I would try to avoid this complication if possible.
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.
If you want to, you can overwrite attributes.
This is a good point. I buy the logic that conflicts are unlikely to be common, and thus that this proposal is going to work reasonably well in most situations. If a conflict does arise, the user has an escape hatch with the ability to wrap the exporter with logic that adds a prefix to the key in conflict.
attribute2: scope-attribute-2 # overwrites attribute2 on resource | ||
attribute3: resource-attribute-3 # from the resource, not overwritten | ||
attribute4: data-point-2-attribute-4 # overwrites attribute4 from the scope | ||
``` |
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.
Asking just for my understanding. Can you list a real life example of an attribute that could be present both at the resource and at the level of a span/metric/log_record?
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.
One thing that comes to mind is a responsible_team
. In the resource attribute, you would set the department that is responsible for the respective product, and you can overwrite it on Spans/Metrics if the code producing them is maintained by a specific team within that department. That way, the resource would be the "fallback" but you can be more specific if you want to.
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.
Thanks, I used to think we will never have same attribute in both the resource and the signals since they serve a different purpose, but looks like there can be a few valid cases.
I want to question and understand your example though. The team responsible for running my service pod could be devops (specified at resource level), but the team responsible for the application is an engineering/dev team (specified in the span). Don't you want to capture both?
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.
I agree, that they should never overlap, but since its possible to put whatever you want on the attributes it might happen.
For the example, I thought about it more like the team that developed the code. The team running the code is separate in my opinion. responsible_team
might not be the best name for the attribute, maybe separate attributes like dev_team
and devops_team
would be better if you want to distinguish them.
I was thinking of an attribute value of responsible_team=dev
(or dev_team=dev
) on the resource for all the code that was written before instrumentation was added. All code written (and instrumented) later will add the actual team as an attribute on the span/metric/log (e.g. responsible_team=java-service-team
/dev_team=java-service-team
).
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.
@scheler does this answer your question?
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.
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
### Traces | ||
|
||
``` | ||
Span.attributes > ScopeSpans.scope.attributes > ResourceSpans.resource.attributes |
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.
I haven't thought of doing renaming instead of overwriting, but it is worth considering as an option.
A couple thoughts that may help:
- Attribute conflicts are likely going to be very rare. AFAIK, there isn't currently any real semantic convention that uses the same attribute name on different levels.
- There are existing implementations that flatten and overwrite, see e.g. Zipkin exporter in Collector so we have a precedent.
- I do like the fact that prefixing makes flattening a non-destructive operation. However, in real-world use cases you are likely mostly interested in the most specific value of the attribute, recorded at the innermost level, so overwriting seems to be a natural behavior.
If we could have real examples where attribute names can conflict it would help to make a decision. The responsible_team
example I is confusing because we do have a convention for that already and it is called service.namespace
and is supposed to be recorded on the Resource only. Any other examples that we can look at?
If opinions are split on this it is also possible to make this behavior configurable (i.e. to overwrite or to prefix) but I would try to avoid this complication if possible.
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
non-hierarchical representation (e.g., Prometheus/OpenMetrics labels). | ||
In the case of OpenMetrics, the set of labels is flat and must have unique | ||
labels only | ||
(<https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#labelset>). |
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.
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Not stale |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Fixes #2535
Changes
Adds a new document, attribute-precedence.md, which is intended to give guidance on how to flatten out attributes when transforming OTLP to a flat set of attributes with unique keys (e.g. Prometheus/OpenMetrics labels).
This document is intended as a supplementary guideline.
Note that the guidelines are valid for semantic convention attributes as well as for custom attributes, where producers can put whatever they want. These custom attributes might overwrite other custom attributes or semantic convention attributes.
Related issues #2535