profiles: Add comments about 1-indexing and 0 meaning unset

[brancz]

As discussed in the last otel profiling call.

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

(feel free to tag anyone else)

[christos68k]

LGTM

[jhalliday]

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

[florianl]

While I agree with the intention, I'm not sure if this is the correct way to express it in gRPC.

message Location should either have mapping_index set or make use of line - I'm not sure if there are valid cases where it makes sense to set both, so I'm skipping this case here for simplicity.

If mapping_index is not set, then address should also not be set, as address can not map to something within [Mapping.memory_start...Mapping.memory_limit].
Does it make sense to make mapping_index, address and line optional parameters and request (maybe by comment), that either mapping_index (and optionally address) or line is set?

Setting function_index in Line to 0 feels like a work around that should be improved.

[brancz]

If the goal is to be pprof-compatible then we do have to do this, and I don't feel like this tiny semantic is where we should draw the line. Besides, I don't think we can predict all the useful combinations, we should just be able to say something is unset or not.

I think @petethepig brought up the case yesterday that it's not entirely unreasonable that there could be an agent that always performs local symbolization even of native frames (I believe the Grafana Pyroscope Agent does this, but I'm unsure about whether it sets mapping and address as well).

[felixge]

I agree that for now we should be pprof compatible, so we should go ahead and make this change.

@florianl I'm a little confused your comment. Are you suggestion that there is a difference between mapping_index being set to 0 vs not being set at all? My understanding is that protobuf does not make such a distinction (link).

[florianl]

Are you suggestion that there is a difference between mapping_index being set to 0 vs not being set at all?

Yes, I think there is a difference. With the current situation mapping_id is not optional and needs to be set. That is why setting it 0 to indicate that it is unset, feels like a workaround to me. There is a optional field label, but it is not used here or suggested.
Currently, receivers of this signal should always expect a value for mapping_id, which is not possible in every situation.

[brancz]

I don't understand what the difference is, I mean understand the optional keyword exists in protobuf and is not used, but functionally. pprof is what it is and the goal is to be pprof compatible, and the mapping ID being 0 is how pprof decided to say mapping is unset. I get that the optional field could have been a good use here but using it now would break pprof compatibility.

[florianl]

Personally, I think, using optional field label would be a nicer way and would still be pprof compatible. Using 0 to indicate something is not set feels like a silent agreement for pprof, for which I did not find documentation.

[felixge]

Thanks, I took a closer look at the optional keyword. Yes, that would be nice here. But as @brancz points out, we can't change this without breaking pprof compatibility, so it's a no from me for now. We can revisit this if the discussions with Google about the future of otel/pprof don't pan out.

for which I did not find documentation

I think we should consider the pprof command line tool to be the canonical interpretation of the semantics of the profile.proto format when the docs are unclear.

[felixge]

I agree that for now we should be pprof compatible, so we should go ahead and make this change.

@florianl I'm a little confused your comment. Are you suggestion that there is a difference between mapping_index being set to 0 vs not being set at all? My understanding is that protobuf does not make such a distinction (link).

[tigrannajaryan]

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

[tigrannajaryan]

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

[tigrannajaryan]

Is this PR still in progress?

[aalexand]

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]

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