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: Add comments about 1-indexing and 0 meaning unset #554

Closed brancz closed 2 months ago

brancz commented 6 months ago

As discussed in the last otel profiling call.

@felixge @florianl @petethepig @tigrannajaryan @jack-berg @simonswine

(feel free to tag anyone else)

jhalliday commented 6 months ago

Good Idea! Possible enhancements:

For the strings table there is a comment at point of declaration 'string_table[0] must always be "".' and likewise for the mapping tables 'mapping[0] will be the main binary.' (which may be construed as inconsistent with '1-indexed')

It would be useful I think to add similar at point of declaration for the function table and link table, which are likewise cases where a referring field is optional at the spec level but not at the protocol encoding level, thus leaving '0' as a problematic value, then also add similar comments at point of use

tigrannajaryan commented 6 months ago

@open-telemetry/profiling-maintainers you can ping me when the PR is ready to be merged (all conversations must be first resolved).

tigrannajaryan commented 3 months ago

@open-telemetry/profiling-maintainers this PR appears to be stale. Please update/resolve comments so that we can merge or close.

tigrannajaryan commented 2 months ago

Is this PR still in progress?

aalexand commented 2 months ago

This PR is obsolete as is and should be closed.

There is no plan currently to make the indexing one-based in OLTP profiling format. It is going to be zero-based, a direct index into the corresponding array.

What was discussed in one of the SIG meetings here though is how to express nil references and the tentative agreement I think was to have a convention that the zeroth element in all arrays will be effectively empty / unset, so when the index reference is zero this effectively means null reference. But the array would still have that element, and there is index shift-by-1 in the addressing.

So I think this PR talks about an important aspect of the profiling format and the current discussion is somewhat along the same lines, but given that there is no pprof compatibility-at-the-wire format requirement anymore I think we should close this PR and tackle this aspect separately.

tigrannajaryan commented 2 months ago

Closing. Please re-open / create new PR as needed.