Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 havemapping_index
set or make use ofline
- 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, thenaddress
should also not be set, asaddress
can not map to something within[Mapping.memory_start...Mapping.memory_limit]
.Does it make sense to make
mapping_index
,address
andline
optional parameters and request (maybe by comment), that eithermapping_index
(and optionallyaddress
) orline
is set?Setting
function_index
inLine
to0
feels like a work around that should be improved.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 to0
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 it0
to indicate that it is unset, feels like a workaround to me. There is aoptional
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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.