-
Notifications
You must be signed in to change notification settings - Fork 258
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
profiles: align type of index into string table #557
Conversation
With `Mapping.filename`, `Function.name`, `Label.key` and others the type of the index into the string table is always of type `int64`. For consistency align the type of `Location.type_index`, which is also an index into the string table, to `int64`.
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.
Works for me. There might be some concerns around memory usage here, but we can come back to this if it turns out to be problematic.
Does it still meet the requirement of "every valid pprof profile is also a valid Otel profile"? |
@tigrannajaryan - yes it does. As |
Love this change! – are there any blockers to iron out before it can be merged? |
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. |
With
Mapping.filename
,Function.name
,Label.key
and others, the type of the index into the string table is alwaysint64
. For consistency align the type ofLocation.type_index
, which is also an index into the string table, toint64
.type_index
is an element introduced by the OTel Profiling SIG and was added tomessage Location
. Changing this type does not break compatibility with the original pprof, as the original pprof does not have this element inmessage Location
.FYI: @petethepig @open-telemetry/profiling-maintainers