null attributes that have been previously set results in the attribute being cleared and the key being dropped from the attribute set

[codeboten]

To be compliant with the tracing spec: null attributes that have been previously set results in the attribute being cleared and the key being dropped from the attribute set

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes

[codeboten]

Looks like the spec moved the relevant details here: https://github.com/open-telemetry/opentelemetry-specification/blob/671f5abe0eb99451f86df6da8aa7cd4b14d22633/specification/common/common.md#attributes

[LetzNico]

Hi there!

I would like to get started contributing on the repo. This is what I understood from the specs and the issue:

• An attribute's value should never be None
• Adding an attribute with a None value should be a no-op or fire a warning
• The scope of this issue is to implement the following mechanism: modifying the value of an existing attribute from a valid value to None should pop the attribute

Can you confirm that I understood correctly? If yes, I'm down to write a PR 🙂

[lzchen]

• An attribute's value should never be None

See below.

• Adding an attribute with a None value should be a no-op or fire a warning

Possibly. Maybe we can just let this occur and then the exporter deals with the value accordingly. Should be fine to implement it as a no-op though.

• The scope of this issue is to implement the following mechanism: modifying the value of an existing attribute from a valid value to None should pop the attribute

Yes.

[LetzNico]

Thanks for your answer :). Gonna work on this and get back to you soon

[LetzNico]

Looks like the spec has changed regarding that topic: open-telemetry/opentelemetry-specification#992 defines updating an attribute value to null to be undefined behavior (disc here open-telemetry/opentelemetry-specification#797). In the current implementation, this is a no-op. According to this comment, it is acceptable.
I suggest to keep the code as is, ditch the PR and close the issue. What do you think?

[lzchen]

@LetzNico
I believe your understanding is correct, we can leave the no-ops for setting attributes to None.

However, I think due to this logic, we still have the issue of None attributes in a Sequence type being dropped.

[LetzNico]

@LetzNico
I believe your understanding is correct, we can leave the no-ops for setting attributes to None.

However, I think due to this logic, we still have the issue of None attributes in a Sequence type being dropped.

That is to comply this part of the spec, right?

null values within arrays MUST be preserved as-is (i.e., passed on to spanprocessors / exporters as null). If exporters do not support exporting null values, they MAY replace those values by 0, false, or empty strings.
This is required for map/dictionary structures represented as two arrays with indices that are kept in sync (e.g., two attributes header_keys and header_values, both containing an array of strings to represent a mapping header_keys[i] -> header_values[i]).

If yes, I've updated the PR, good for review :). Thanks for your help!