Skip to content
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

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

florianl
Copy link
Contributor

@florianl florianl commented May 8, 2024

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

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`.
@florianl florianl requested review from a team May 8, 2024 07:35
Copy link
Member

@felixge felixge left a 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.

@tigrannajaryan
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

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

@florianl
Copy link
Contributor Author

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.

@tigrannajaryan tigrannajaryan merged commit 14afbd4 into open-telemetry:main Aug 13, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants