search

open-telemetry / opentelemetry-proto

OpenTelemetry protocol (OTLP) specification and Protobuf definitions
https://opentelemetry.io/docs/specs/otlp/
Apache License 2.0
607 stars 262 forks source link

profiles: align type of index into string table #557

Closed florianl closed 3 months ago

florianl commented 6 months ago

With Mapping.filename, Function.name, Label.key and others, the type of the index into the string table is always int64. For consistency align the type of Location.type_index, which is also an index into the string table, to int64.

type_index is an element introduced by the OTel Profiling SIG and was added to message Location. Changing this type does not break compatibility with the original pprof, as the original pprof does not have this element in message Location.

FYI: @petethepig @open-telemetry/profiling-maintainers

tigrannajaryan commented 6 months ago

Changing this type does not break compatibility with the original pprof, as the original pprof does not have this element in message Location.

Does it still meet the requirement of "every valid pprof profile is also a valid Otel profile"?

florianl commented 6 months ago

Does it still meet the requirement of "every valid pprof profile is also a valid Otel profile"?

@tigrannajaryan - yes it does. As type_index was introduced by the OTel Profiling SIG it does not affect original pprof profiles. In general original pprof profiles use int64 as index type and this PR aligns the OTel Profiling SIG introduced element to also use int64 as its type.

javierhonduco commented 4 months ago

Love this change! – are there any blockers to iron out before it can be merged?

florianl commented 4 months ago

friendly ping @open-telemetry/specs-approvers - can this change get merged?

@felixge , @petethepig and @christos68k from the @open-telemetry/profiling-maintainers group approved this change already.