profiles: move attribute_table to message ScopeProfiles #609
Conversation
Move `attribute_table` from `message Profile` to `message ScopeProfiles`. This reduces the amount of duplicate attributes significantly if more than one kind of profile is reported - e.g. on CPU and memory heap profiling data. ping @open-telemetry/profiling-maintainers
| @@ -144,6 +144,9 @@ message ScopeProfiles { | |||
| // https://opentelemetry.io/docs/specs/otel/schemas/#schema-url | |||
| // This schema_url applies to all profiles in the "profiles" field. | |||
| string schema_url = 3; | |||
|
|
|||
| // Lookup table for attributes. | |||
| repeated opentelemetry.proto.common.v1.KeyValue attribute_table = 4; | |||
There was a problem hiding this comment.
The argument can be made to even move attribute_table to ResourceProfiles or ProfilesData instead of ScopeProfiles. If you prefer this, please let me know.
There was a problem hiding this comment.
ProfilesData doesn't seem like a good fit as if my understanding is correct it can be populated by intermediate processors that receive data from multiple sources.
ResourceProfiles can also contain data from multiple instrumentation scopes so we'd be introducing implicit merging of key values requirement if we moved it there.
Doesn't the same reasoning (reduce amount of duplicate data) also apply to moving the string table?
There was a problem hiding this comment.
Also, should we merge #606 into this PR, otherwise it can be a little confusing (e.g. doesn't update Profile attributes to be references)
There was a problem hiding this comment.
Not moving attribute_table for the named reason to ProfilesData or ResourceProfiles sounds good to me. When moving "up" attributes_table to ScopeProfiles I just looked at it from a protocol perspective and not from a potential synchronization issues of processors.
Doesn't the same reasoning (reduce amount of duplicate data) also apply to moving the string table?
yes - the same applies to string_table. I can add it here as well.
Also, should we merge #606 into this PR, otherwise it can be a little confusing (e.g. doesn't update Profile attributes to be references)
To me, these are independent proposals that should be handled and decided on separately. So I didn't merge everything into a big PR. Depending on which PR gets merged first, the other still can be updated.
There was a problem hiding this comment.
Moved string_table with 55bbd73 as well to ScopeProfiles.
Signed-off-by: Florian Lehner <[email protected]>
|
Putting this PR on hold for the moment. |
Move
attribute_tablefrommessage Profiletomessage ScopeProfiles. This reduces the amount of duplicate attributes significantly if more than one kind of profile is reported - e.g. on CPU and memory heap profiling data.ping @open-telemetry/profiling-maintainers