profiles: Add comments about 1-indexing and 0 meaning unset #554
Conversation
|
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 |
| @@ -329,7 +329,7 @@ message Location { | |||
| uint64 id = 1; | |||
| // The index of the corresponding profile.Mapping for this location. | |||
| // It can be unset if the mapping is unknown or not applicable for | |||
| // this profile type. | |||
| // this profile type. 1-indexed, 0 indicates unset. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -329,7 +329,7 @@ message Location { | |||
| uint64 id = 1; | |||
| // The index of the corresponding profile.Mapping for this location. | |||
| // It can be unset if the mapping is unknown or not applicable for | |||
| // this profile type. | |||
| // this profile type. 1-indexed, 0 indicates unset. | |||
There was a problem hiding this comment.
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).
|
@open-telemetry/profiling-maintainers you can ping me when the PR is ready to be merged (all conversations must be first resolved). |
As discussed in the last otel profiling call.
@felixge @florianl @petethepig @tigrannajaryan @jack-berg @simonswine
(feel free to tag anyone else)