profiles: make profile_id optional
#665
Conversation
There was a problem hiding this comment.
I think it is a good idea to make this field optional. Speaking for OTel eBPF profiler it is just a randomly generated value. So based on the profile_id reported by OTel eBPF profiler, no conclusions can be drawn to the actual data it reports.
|
Markdown lint failed (unrelated to changes in this PR). Please file an issue to fix it. |
I proposed a fix in #673 |
| // all zeroes is considered invalid. | ||
| // | ||
| // This field is required. | ||
| // all zeroes is considered invalid. It may be used for deduplication and signal |
There was a problem hiding this comment.
if the ID is a 16-byte array, and is unset, then all bytes are set to zero - is a profile ID unset therefore invalid per this comment?
There was a problem hiding this comment.
Unset will look like "field not set" (e.g. for Python message.HasField("profile_id") == false) or an empty byte array depending on the Protobuf implementation. But in both cases, unset won't look like a 16-byte array of zeros.
There was a problem hiding this comment.
I think the idea behind stating that An ID with all zeroes is considered invalid is just to provide a null value that the community can use in implementations when a well-formed value must be set for practical reasons.
| // correlation purposes. It is acceptable to treat two profiles with different values | ||
| // in this field as not equal, even if they represented the same object at an earlier | ||
| // time. | ||
| // This field is optional; an ID may be assigned to an ID-less profile in a later step. |
There was a problem hiding this comment.
What value is it set to if unset and optional?
There was a problem hiding this comment.
There was some discussion in #652 about this, the idea is that if a profile does not have an ID it may receive one for deduplication purposes later, e.g. in the collector. The value would be a real valid identifier, including 16 zeros if that makes sense.
There was a problem hiding this comment.
the proto file comment says 16 zeros is invalid. I cannot reconcile it with your statement above.
There was a problem hiding this comment.
Right, that's a bit unclear, let me rephrase.
The value would be a real valid identifier, including 16 zeros if that makes sense.
The receiver of an id-less profile (i.e. profile_id absent) is allowed to assign an arbitrary value to profile_id, including the null value (16 zeros).
I'm not sure whether the semantic difference between unset and 16 zeros is part of the spec though. I'm just guessing.
Fixes #652