Review HexString func on SpanID and TraceID

[bogdandrutu]

Feedback from reviewing contrib usages:

1. Lots of usages in logs zap.String("trace_id", sp.TraceID(), this has unnecessary overhead when the level is low (like Debug) since the conversion to string will happen. I would propose to offer a String() method that can be used instead as zap.String("trace_id", sp.TraceID() - not sure if we should return empty string for "empty" span, but we can debate that.
2. Lots of places where we check either if the SpanID is not empty, or we check if the result of HexString is not empty string then use it. We can replace these with a check if the ID is not Empty then use hex.EncodeAsString(id[:]).

I am proposing the following plan:

1. Add String func with the same behavior as current HexString which returns empty if the ID is empty and deprecate the current HexString.
2. Fix all usages based on the initial feedback that I found.
3. Analyze what are the other usages and avoid depending on String() func if possible for conversion, instead use hex.EncodeAsString.

[dmitryax]

@bogdandrutu just to clarify, are you suggesting that we rename HexString to String to avoid unnecessary bytes copying and encoding in debug logs, recommend the String function only for displaying purposes, and ask users to use hex.EncodeToString if they need the hex string representation as an identifier, right?

[dmitryax]

Also zap.String takes string as an argument, so we will need to replace them to zap.Any to supply a Stringer instance.

[mx-psi]

Also zap.String takes string as an argument, so we will need to replace them to zap.Any to supply a Stringer instance.

We can use zap.Stringer, right?

[dmitryax]

We can use zap.Stringer, right?

Right 👍

[bogdandrutu]

Also zap.String takes string as an argument, so we will need to replace them to zap.Any to supply a Stringer instance.

Yes, the idea was to use zap.Stringer once you implement String() method. This way the String() func will be called only if the log is recorded.

recommend the String function only for displaying purposes, and ask users to use hex.EncodeToString if they need the hex string representation as an identifier, right?

Correct.

[dmitryax]

@bogdandrutu can you also please clarify why we can't we keep HexString in addition to String to promote consistent representation of trace_id/span_id in pdata instead of asking users to call hex.EncodeTosString?

[bogdandrutu]

Will be duplicate code, also not sure we are the entity to ensure "consistent representation". It is also different than the w3c representation since we don't represent empty as 0000000000000000 but as empty string.

[bogdandrutu]

Also, https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/otlp.md#json-protobuf-encoding does not clarify how to represent empty IDs.

[bogdandrutu]

So the concern is that we made a decision there, that is not based on any standard to represent empty ID as empty string, because of that I feel not good exposing this in a public function that others rely on. Will take long time to make a decision in specs, so better not to expose this as non "debug" only functionality.

[dmitryax]

Ok, makes sense