Label with zero int64 value and no unit get dropped during decoding

[ms-hujia]

The current behavior of Parse() function will drop labels with zero int64 value and no unit. For example, a thread id with zero value is meaningful and no unit is applicable. This behavior will cause the thread id information getting dropped for specific samples since the key won't exists in the NumLabel map of decoded Sample.

Should this be fixed to support label with zero int64 value?

[gmarin13]

If you have labels with numeric values, you just need to add units to them to mark them as valid. You can specify the string ID you used for the label to the unit field as well.

[ms-hujia]

Hi @gmarin13, I know this is one of the workaround. Another option is to store the value as string. As I explained in the description, sometimes unit is not applicable to specific kind of label (e.g., thread id). Also applying any of these workarounds is not zero cost.

Basically, this issue comes from the behavior of handling a zero num value in Label is not well documented in protobuf definition or any markdown file in this repo.

[aalexand]

Last time I looked at it, this seemed somewhat non-trivial to fix without disturbing backward compatibility or without adding disproportional amount of code / complexity for the size of the problem. We usually add a unit in these cases and move on, so current plans to fix it. I am also not sure this belongs in the profile.proto documentation since formally speaking this behavior is not a part of the format.

[aalexand]

If there is a simple small fix that can be clearly covered by tests and clearly won't break anyone I'd be happy to review a PR.

[ms-hujia]

Hi @aalexand, did you mean there's NO plan according to some previous discussion? Could you share any link or keyword if it's still on your mind?

[ms-hujia]

Regarding backward compatibility, my fix #1003 will change the behavior of any server implementation which receives pprof data in protobuf format and parses it to object in memory. So,

1. If the maintainer has been aware of the mentioned behavior, they should have applied some workaround. Taking Parca as an example, they used string label to store thread id and finally they deprecated pprof totally for other reason.
2. If the maintainer is still not aware of this behavior and the server do accept a zero int64 value for the label, my fix should make their code work as expected since the zero value label is back.
3. If the maintainer is still not aware of this behavior and the server doesn't accept a zero int64 value for the label, that's the concern here. If they don't check the zero value, the server will panic if they use the value as divisor after updating pprof. The problem here is they implicitly depends on the undocumented behavior here.