profiles: mark Line.function_index optional #621
Conversation
Similar to Location.mapping_index also mark Line.function_index optional. Signed-off-by: Florian Lehner <[email protected]>
| // Line number in source file. | ||
| int64 start_line = 4; |
There was a problem hiding this comment.
Can we make a note here? 0 if not available/making it optional?
There was a problem hiding this comment.
@florianl I thought the consensus in the last SIG was that we'd use optional everywhere?
There was a problem hiding this comment.
My understanding was, that we use optional for function_index, similar to optional int32 mapping_index = 1; to have consistency. And improve on the documentation of the fields within message Function, pointing to the special meaning of Profile.string_table[0], if there is no value.
But i'm happy to also mark all fields within message Functionwith optional.
There was a problem hiding this comment.
I don't see it written down in the agenda but I do remember the point of "specialness" for tracking a missing value for strings was raised and the final consensus was to use optional. Maybe @felixge can chime in.
There was a problem hiding this comment.
Unfortunately I don't recall if we decided to change how we handle strindex fields (we should capture notes better), but thinking more about this I would propose we leave them without optional keyword and with special zero index value meaning empty for now. We can discuss later.
Motivation: optional field at least in Go are encoded at runtime as a field by pointer rather than by value, so if we make them all optional we end up with a lot more "*int32" fields instead of "int32". These dynamic allocations can add up and have memory usage and GC CPU costs.
| int32 filename_strindex = 3; // Index into string table | ||
| // Index into Profile.string_table to the name of the function, in | ||
| // human-readable form. | ||
| // Use index 0 if value is not available. |
There was a problem hiding this comment.
Describing the semantics of how we index strings at every field is becoming repetitive IMO.
For *_strindex fields I would suggest we focus the documentation on the semantics of this string field. The strindex itself points to the fact that this is a string index, with the same meaning universally (including index 0 for empty value).
So, maybe put the documentation simply as
// Function name. Empty string if not available.
int32 name_strindex = 1;
// Function name, as identified by the system. For instance,
// it can be a C++ mangled name. Empty string if not available.
int32 system_name_strindex = 2;
// Source file containing the function. Empty string if not available.
int32 filename_strindex = 3;
...
?
| // Line number in source file. | ||
| int64 start_line = 4; |
There was a problem hiding this comment.
Unfortunately I don't recall if we decided to change how we handle strindex fields (we should capture notes better), but thinking more about this I would propose we leave them without optional keyword and with special zero index value meaning empty for now. We can discuss later.
Motivation: optional field at least in Go are encoded at runtime as a field by pointer rather than by value, so if we make them all optional we end up with a lot more "*int32" fields instead of "int32". These dynamic allocations can add up and have memory usage and GC CPU costs.
|
The Profiling SIG (2025-02-20) wants to explore the protobuf Opaque API first - before making further steps. |
Similar to Location.mapping_index also mark Line.function_index optional.
For visibility: @open-telemetry/profiling-maintainers