profiles: Make mapping in Profile optional #556
Conversation
As commented in [0] and discussed in the OTel Profiling SIG meeting, there are situations where a main binary for a Profile can not be identified. For these cases mark the field optional. [0]: open-telemetry#534 (comment) Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
opentelemetry/proto/profiles/v1experimental/pprofextended.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
| @@ -79,6 +79,8 @@ message Profile { | |||
| repeated Sample sample = 2; | |||
| // Mapping from address ranges to the image/binary/library mapped | |||
| // into that address range. mapping[0] will be the main binary. | |||
| // If multiple binaries contribute to the Profile and no main | |||
| // binary can be identified, mapping[0] has no special meaning. | |||
There was a problem hiding this comment.
One aspect to discuss I think is how pprofextended expresses nil references to entities like mapping. Locaiton.mapping_index is an index and it's not marked as "optional" proto3 field, so there there is no presence bit. Which means that if the mapping array is empty and mapping_index is not set then effectively it's zero and effectively it's an out of bound reference. Are we saying that any out of bound reference is considered a nil reference? I think this needs to be discussed and spelled out. Because an alternative would be have a mapping entity at index zero but have it distinguishably empty - no name, all fields are zero etc.
Note that in the original pprof proto this is handled differently since the field is Location.mapping_id (rather than mapping_index), so the references were through this additional ID indirection and it is required that the IDs are positive integers, so the ID of 0 is unambiguously a nil reference.
There was a problem hiding this comment.
I agree that the non existence of an element, is not well defined in both protocols. The intention of this PR is to improve on that front for pprofextended.
I might have missed the part, where the original pprof requires _id to be a positive value (which includes the implicit 0 value to be interpreted as non existence?)
While the original pprof uses _id the naming changed to _index with pprofextended.
Because an alternative would be have a mapping entity at index zero but have it distinguishably empty - no name, all fields are zero etc.
From my perspective that is the best way to continue, while keeping backwards/forwards compatability between pprof and pprofextended.
As commented in 0 and discussed in the OTel Profiling SIG meeting, there are situations where a main binary for a Profile can not be identified. For these cases mark the field optional.
FYI: @brancz @petethepig @open-telemetry/profiling-maintainers