Opentelemetry-java: RecordException overwrites additional attributes with generated ones instead of the other way round.

[Oberon00]

See code here:

https://github.com/open-telemetry/opentelemetry-java/blob/0024aa9beeafd512c935b5378d031963e0d4675b/sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java#L440-L442

Spec at https://github.com/open-telemetry/opentelemetry-specification/blob/a6db328ada9bf30ba880b9cfab3296787c39fe67/specification/trace/api.md#record-exception

If attributes with the same name would be generated by the method already, the additional attributes take precedence.

Also, I wonder if we should still record any additional attributes if exception is null.

[jkwatson]

~@Oberon00 I think it's even a bit worse than the title of the bug. Currently, Attributes are not deterministic in which one will win, when the same key is provided. So, definitely an issue, and one that's been lurking for a bit. thanks for pointing out the spec that we're not in line with.~

[dengliming]

I'm a little confused about take precedence. Does this mean that Attributes that the method has already generated can be overridden by additional Attributes?

[Oberon00]

Exactly. Would changing the spec to "Attributes MUST NOT be added to the event by RecordException if an attribute with the same key is in the additional attributes." be more clear? Actually it has to be "Additional attributes MUST be set unchanged on the event even if an attribute with the same key would be generated by RecordException." But I feel that would be a bit convoluted a way of stating it.
Note that you can even optimize this to e.g. not capture a stacktrace if the additional attributes already contain one (provided your attributes provide an efficient membership test).

[dengliming]

SemanticAttributes.EXCEPTION_TYPE.set(attributes, exception.getClass().getCanonicalName()); I'm a little confused about what's wrong with this code. if the additional attributes already contain EXCEPTION_TYPE attribute, and can't be changed. right?

[jkwatson]

~@Oberon00 I think it's even a bit worse than the title of the bug. Currently, Attributes are not deterministic in which one will win, when the same key is provided. So, definitely an issue, and one that's been lurking for a bit. thanks for pointing out the spec that we're not in line with.~

I take it back. The way the quicksort is implemented, the first-in key always wins when there are duplicate keys. So, it is deterministic, and the first key set will be the winner. So, I think that the current implementation is correct, because the Attributes that are passed in are used as the original source of attributes. If the method tries to "overwrite" them, those values will be ignored. We should add a test to confirm this; I will do so.

[Oberon00]

Oh, I just assumed it worked like a regular map, instead of the other way round. If I call SetAttribute on the same span multiple times with the same key, will the last key still win?

[jkwatson]

Oh, I just assumed it worked like a regular map, instead of the other way round. If I call SetAttribute on the same span multiple times with the same key, will the last key still win?

SetAttribute on the Span is a little different, since that doesn't use the Attributes implementation. I believe last-one-in wins on the span attributes, but first-one-in wins on Event attributes. I don't think it's a good thing that there's a difference in the behavior!

Also, there is already a test that the provided attributes over-ride the "instrinsic" attributes: https://github.com/open-telemetry/opentelemetry-java/blob/ed08e9f28783978e792156fa97283b384e90bf87/sdk/tracing/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java#L753

[Oberon00]

OK, then it seems this bug report is invalid. Sorry, I just read the code and assumed it was an obvious oversight.