-
Notifications
You must be signed in to change notification settings - Fork 303
[resource] introduce reference based attributes #733
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
base: main
Are you sure you want to change the base?
Conversation
Reduce duplicate entries by using a reference based approach in combination with a lookup table. Signed-off-by: Florian Lehner <[email protected]>
|
I have 2 questions:
message AnyValue {
oneof value {
...
uint32 string_ref = 8;
}
} |
|
I would like to see some benchmarks showing the impact of the change on profiling signals. I think we want to see the following with and without ref-based attributes on a few production-like payloads:
This is a pretty significant change so I want benchmarks to support the change. |
| int32 key_index = 1; | ||
|
|
||
| // Reference to the value of the pair in ProfilesDictionary(?).string_table. | ||
| int32 value_index = 2; |
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.
Do we want the value type to be restricted to strings? If not, we should probably do it like we do for the attributes in profiling?
Of course this means no dictionary for the values, but they're supposed to be low cardinality.
| int32 value_index = 2; | |
| opentelemetry.proto.common.v1.AnyValue value = 2; |
Edit: I see @tigrannajaryan asked similar questions above.
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.
Just wanted to get the discussion started.
The recommendation in #733 (comment) sounds promissing:
message AnyValue {
oneof value {
...
uint32 string_ref = 8;
}
}
I think, we definitely should also reference string values.
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.
Yeah, giving AnyValue the ability to reference into the string table would be useful. But it seems like that would be an additional step we could take that doesn't need to be in the scope of this PR? Especially if we apply the change I suggested.
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.
Updating AnyValue with the ability to use a reference as a next step sound good to me. Applying the changes to message Resource in a first step limits also the number of follow up changes in other components, like OTel collector or SDKs.
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.
Applying the changes to message Resource in a first step limits also the number of follow up changes in other components, like OTel collector or SDKs.
I disagree that this is a good approach. It creates an intermediary state that is going to be short-lived, which we have to get rid of once AnyValue gains ref capability. So the anticipated changes to the protocol look like this:
- Add ref attributes to Resource.
- Add ref to AnyValue.
- Deprecate ref attributes in Resource, but keep supporting both refs.
- Wait for a few years for the next major version of OTLP where we can make breaking changes.
- Delete ref attributes from Resource.
Consider instead this:
- Add refs to AnyValue. Done.
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.
@tigrannajaryan applied the requested changes with c8b22c6.
Agreed. @Gandem and I started working on this yesterday. Our rough plan is the following:
We hope to have it done by the Nov 13 profiling SIG meeting. |
@felixge My old bench from early OTLP designs is here: https://github.com/tigrannajaryan/exp-otelproto Feel free to borrow anything if there is any useful/relevant stuff there. Might be outdated/bitrotten since it's been a while. |
| int32 key_index = 1; | ||
|
|
||
| // Reference to the value of the pair in ProfilesDictionary(?).string_table. | ||
| int32 value_index = 2; |
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.
Yeah, giving AnyValue the ability to reference into the string table would be useful. But it seems like that would be an additional step we could take that doesn't need to be in the scope of this PR? Especially if we apply the change I suggested.
…ct_attributes-field-name add missing dict_attributes field name
| // Note: This is currently used exclusively in the Profiling signal. | ||
| // | ||
| // Status: [Development] | ||
| repeated StringKeyValue dict_attributes = 4; |
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 happens if the same key is present here and in attributes? Do we need precedence rules? Is it invalid state?
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.
My suggestion is that if there are elements in dict_attributes, then attributes must not hold any element.
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.
Let's add that as an invariant in the comments then
tigrannajaryan
left a comment
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.
Blocking until we have clarity on the way forward.
Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
|
@open-telemetry/profiling-maintainers I would like to see a proper OTEP for this change. It should include the following:
|
Ack. Working on it.
Can you sync with @jsuereth on this? We discussed this with him in the last SIG meeting and he didn't see large obstacles, e.g. the need for capability negotiation. The rough plan is that the new |
Let's set aside what happens inside the Collector for a moment. I am looking at a scenario where a pair of OTLP sender and receiver of different versions try to communicate (can be SDK, Collector, a backend, etc). What happens if the OTLP sender uses the new version of protocol and encodes some attributes via dictionary, however the receiver is an old version and does not know anything about dictionaries? The receiver will see I do not think this is acceptable.
@jsuereth let's discuss in spec meeting today if you are around. @felixge and other @open-telemetry/profiling-maintainers it would be good if you can join too. |
| // Set of attributes that describe the resource. | ||
| // Key and Value combination MUST be unique. | ||
| // attributes MUST NOT be used if dict_attributes is used. | ||
| // | ||
| // Note: This is currently used exclusively in the Profiling signal. | ||
| // | ||
| // Status: [Development] | ||
| repeated RefKeyValue dict_attributes = 4; |
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 would put the dictionary in the "ResourceLogs/ResourceSpans/ResourceMetrics" and be shared between all attributes coming from a resource?
happy to hear others what they think
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 for the suggestion. Quick context: the technical committee (cc @jsuereth) recommended we follow the OpenTelemetry resource model in the opentelemetry-ebpf-profiler and align with the SDKs’ resource definition.
Today the profiler emits one resource per container (container.id) plus one for everything else. Aligning with the SDKs would mean one resource per process id.
In forking runtimes like Python, a single container often runs many processes. Those processes share almost identical resource attributes (container.id, container.name, etc.). Replicating them across 10 to 20 processes increases the profiling payload, especially the collector’s in-memory footprint.
The benchmarks I’m running with @felixge aim to quantify that overhead and the savings from a dictionary defined at the top level.
If we scope the dictionary to each Resource or inside ResourceLogs/ResourceSpans/ResourceMetrics, it can’t de-duplicate across separate Resource instances once we move to per-process resources. That defeats the goal and wouldn’t prevent the payload growth we’re trying to avoid.
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 we scope the dictionary to each Resource or inside ResourceLogs/ResourceSpans/ResourceMetrics, it can’t de-duplicate across separate Resource instances once we move to per-process resources. That defeats the goal and wouldn’t prevent the payload growth we’re trying to avoid.
Do you think you can run the benchmarks with both approaches (what you suggest and what Bogdan suggests) so that we can see the difference?
| // Note: This is currently used exclusively in the Profiling signal. | ||
| // | ||
| // Status: [Development] | ||
| int32 string_ref = 8; |
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.
Should we use fixed32 instead? It is faster but a bit larger on the wire.
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 think I would keep varint because then we could use lower values for more commonly referenced values and save wire size.
The As far as expanding the usage of this field to other signals, that's indeed a problem. Is this the first time a new field is being added to OpenTelemetry? Is there no prior art?
Sorry I wasn't able to join, but the notes indicate that @florianl and you will work on an OTEP for that? That sounds good, thanks 🙇. I hope to have the new benchmarks soon. |
Signed-off-by: Florian Lehner <[email protected]>
…i2bIKB4hrYZQ6rGE Signed-off-by: Florian Lehner <[email protected]>
|
I have updated this PR to reflect the changes suggested in the OTEP. |
|
@Gandem and I have finished the benchmark we promised. You can view the full results here: The results conclude that this PR is effective and reduces the uncompressed payload size by
A visualization of the
To see the full results, check out this this README. |
|
@felixge these days zstd is more of a standard than the gzip, can you try that as well? |
This is nice because it avoids us having to solve the thorny problems of evolving the stable signals to use dictionaries, but its means that OTLP receivers have to operate in a split brain world, with some signals encoding using dictionaries and others not. This has consequences for things like pdata, which now need to accommodate both worlds. I wonder if the move to reference based attributes in protos, which are occurring here and with entities, are a big enough shift that we should consider moving them to an OTLP v2. |
Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
|
As the tendency is towards |
| // Note: This is currently used exclusively in the Profiling signal. | ||
| // | ||
| // Status: [Development] | ||
| repeated opentelemetry.proto.common.v1.KeyRefValue dict_attributes = 4; |
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 would the performance look like if you used KeyValueAndUnit refs here (where unit is always empty) to see if this saved more data?
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 don't have data to back this, but I assume no difference to the benchmark results we see with #733 (comment).
As most of the time, the additional Unit field is not set it will not cause an overhead. And for the time it is actually set, it will be neglectable.
The Profiling SIG introduced KeyValueAndUnit as we see the need to assign units to values. As an additional field, like Unit, is just an additional and not a breaking change, shouldn't the discussion around Unit separated from the reference based attributes?
In support of open-telemetry/opentelemetry-proto#733 --------- Co-authored-by: Nayef Ghattas <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
I am removing my block since I no longer think I have a better proposal.
| // in which case this AnyRefValue is considered to be "empty". | ||
| oneof value { | ||
| // Reference to the string value in ProfilesDictionary.string_table. | ||
| int32 string_value_ref = 1; |
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.
@bogdandrutu can you please clarify your views on:
- references folded into
AnyValueor introduced with a new messageAnyRefValueas done here? IIRC you expressed a preference for the former in the last Zoom chat we had. @jsuereth and @tigrannajaryan expressed preference for the latter. - references existing in the proto vs pdata? If I'm not mistaken, you've also expressed a preference for the former.
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.
@tigrannajaryan @jsuereth my understanding of the decision is that we will add string_value_ref in the current AnyValue and will comment that is not available for other signals.
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.
add string_value_ref in the current AnyValue
I will update this PR, once @tigrannajaryan @jsuereth also agree on that.
Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
| // Note: This is currently used exclusively in the Profiling signal. | ||
| // | ||
| // Status: [Development] | ||
| int32 string_value_ref = 8; |
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.
Just for me to understand, in comment you suggest uint and here you are using the signed version. What is the expectation?
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.
Sorry for the typo - should be int32. Updated the comment accordingly.
| // Note: This is currently used exclusively in the Profiling signal. | ||
| // | ||
| // Status: [Development] | ||
| int32 string_value_ref = 8; |
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.
As discussed in the SIG call - I'm fine with this as a first step.
I do think you need an update to KeyValue as well where the key is a key_ref, otherwise we aren't solving the bloat problem that motivated this.
Additionaly, just re-expressing my preference that we have a ProfileResource that mirrors (but is different than) Resource and uses a KeyValueRef. We then have the Collector pdata abstract between KeyValueRef/KeyValue and ProfileResource/Resource with the same datastructure to represent both, and serialize apporpriately on the other end.
However, if that's not an option for the collector (due to performance or other concerns) we should move forward with this. cc @bogdandrutu / @tigrannajaryan
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 problem to have "two things" that act the same in golang is that you need to use any (interfaces). Interfaces in golang most of the time require an extra allocation which is unnecessary.
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.
So the option would be to actually behind the scene in the collector generate only one message Resource (and do exactly what I suggest here but hide in the back that there are 2 messages).
My problem with 2 messages is that profiles wants to add Unit for attributes for example and by being different they feel that is ok and added it without making a global decision.
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.
@jsuereth I have added the changes for KeyValue with 2542b13 - hope this works for you?
@bogdandrutu do you have concerns about this?
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.
My problem with 2 messages is that profiles wants to add Unit for attributes for example and by being different they feel that is ok and added it without making a global decision.
I think there's some allowable inconsistency between signals. The decision to have units - I think would be inline w/ a multivariate-metric signal, if we were to add one. Additionally, the decision to add unit was part of the -proto repo, approved by proto maintainers and merged after accepting it as a global decision. Can you clarify how it wasn't? We decided to accept this inconsistency, given trade offs.
If we don't like the inconsitency, then let's propose something that works (or at least, also propose how to deal with all the downsides) of an alternative. Right now, when the two options are "be consistent, but break the ecosystem" or "be inconsistent, but profiling is successful and existing integrations do not break" it's hard to understand the consistency argument.
Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
| AnyValue value = 2; | ||
|
|
||
| // Reference to the string key in ProfilesDictionary.string_table. | ||
| // key MUST NOT be set if key_ref is used. |
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.
NIT: In proto3, wihtout the use of optional, there's not really a way to check "existence" or non-existence. I think we could just say key_ref is not set or the proto3 default value of empty.
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 assume 0 value is invalid for a reference, which makes it pretty easy to check for "existence" without relying on Protobuf's native existence APIs. 0 is also not going to be present on the wire, so it is safe to set it to 0 on the sender's side to indicate that the key is not refcounted.
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.
Right, by convention all dictionary tables use index 0 to indicate 'null' / 'not set'.
Would prefer this sentence rephrased to something like this:
key MUST NOT be set if key_ref is set to a value other than 0.

Reduce duplicate entries by using a reference based approach in combination with a lookup table.
It should help drive the discussion around the use or Resources.
FYI: @open-telemetry/profiling-approvers
We will include a int32 string_value_ref field within the AnyValue message. This allows data points to reference strings via an integer index rather than carrying the full string value.
Collector Efficiency: This is the most pragmatic path to simplify the OpenTelemetry Collector's internal data model (pdata) and streamline code generation.
Consolidation: While creating a separate "Ref" version of AnyValue was considered, it was determined that the confusion caused by a new type would be equal to or greater than simply adding a field to the existing message.
Minimized Impact: In signals where no dictionary is provided (like standard Traces or Metrics), this field has no context and should be ignored, minimizing the risk of accidental misuse.