Define null as a valid attribute value

[arminru]

Resolves #797.
Closes #992.

See #992 for an alternative proposal.

Changes

Specifies null as a valid attribute value.
As a consequence, attempting to set null no longer removes a previously set attribute but overwrites it with null.
Exporters will have to be adapted accordingly. OTLP already supports specifying null values (see common.proto).

[Oberon00]

OTLP already supports specifying null values

The protobuf does but I'm not sure the protocol allows it, i.e., if consumers are prepared.

[bogdandrutu]

@arminru I don't think this resolves the initial issue, for example let's assume an API setAttribute(String key, String value) in java, if the value is null you want this to be propagated (which may seem fine), but then we have another API setAttribute(String key, String[] array_value) if the value is null, are these 2 attributes the same? Type is dropped in this case.

I feel that null as defined in this PR, is a new supported type, not a solution for null types in languages like Java. Am I missing something?

[bogdandrutu]

To clarify, I am not against adding the new type null, but I think just that the issue with null String in Java is not resolved

[Oberon00]

Why not? Do we really need to distinguish null strings from null string arrays?

[Oberon00]

To clarify: Even if we accept nulls, I'm strongly against distinguishing nulls of different types in our data model. Also, even in Java, null instanceof String is false.

[bogdandrutu]

Think about the API not the exporting value (where indeed you probably no longer know if that supposed to be a string or a array). Can you please write the API in Java for example and I will show you that you have 3-4 ways to record null which is strange

[arminru]

@bogdandrutu

You would indeed have multiple options to record a null value in Java but if the doc clearly states that the different types are not distinguished in case of null, I don't see this as problematic. I would rather have the option to pass a null String to the overload accepting Strings and likewise for arrays rather than only accepting null in one single API. This would force users to do something like if (myString != null) setAttribute(key, myString); else setNullAttribute(key);, which does not look preferable to me.
What would you propose instead?
How the Java impl solves this using the AttributeValue class we have there and whether the AttributeType enum would need to be extended by a NULL type is an implementation detail that should be decided by the SIG IMHO.

I think it is fine to state that the type of null (in languages that have this in their type system) is not preserved and will not be distinguished by exporters. I don't see any benefit in preserving the type, if any, for consumers. They either expect a particular type if it's a well-known attribute for which a semantic convention exists, and will know what "kind" of null this is or it's an attribute with unknown semantics anyway and then the type of null won't matter.

[arminru]

@bogdandrutu I stated more explicitly in 8875058 that implementations may and exporters should ignore different types of null. If SIGs decide that they preserve the value for any reason (e.g., because it's just easier to implement or more canonical in that language), it should be fine IMHO but there should not be any need to do so. Exporters must have the option to discard the type, since OTLP, for example, does not support specifying a type when expressing an attribute value of null and it is not expected to have any semantic difference.

[bogdandrutu]

My preference is to try to maybe keep the type if possible (semantic conventions also use types). So having a SetNullAtrribute is fine because we want to support null, but for SetStringAttribute I was thinking to default to empty string if null is passed (like protobufs), and preserve the type.

[Oberon00]

So you would like to change the existing types in the data model to allow an additional value of null, rather than adding a new null type?

[Oberon00]

Note that in JS, Python, Ruby, null/None/nil has it's own type so we would need to add that additionally anyway.

[arminru]

@bogdandrutu Apart from that, what is the motivation of preserving the type?

I'd rather keep the current wording allowing implementations to drop the type. We don't define any semantic difference for the "type" of null, it's not part of the type system in some languages and not supported by OTLP. I also think that - given we deem null a valid value - accepting it for each overload where null can be expressed is just natural and not undesirable.

I don't think we should just assume an empty string if a null string is passed since that does indeed make a semantic difference (value not available/retrieved vs. value is verified to be an empty string). Same for false/0/empty array.

Adding an API SetNullAtrribute for languages that don't support null natively could be an option but I'm not sure if this is necessary or rather a case of YAGNI. This can be discussed separately from this PR, however.

[bogdandrutu]

Let me write down how I read the current specs proposal:

• A primitive type: string, boolean, double precision floating point (IEEE 754-1985), signed 64 bit integer, or null

This will make me thing we need a SetNullAttribute because it is one of the supported value. Independent if the language, because the specs suggest null is one of the possibility.

In languages with type systems where null can be of different types (e.g., a null-valued string or array),
the type of null values MAY be ignored by implementations and different types of null SHOULD NOT be
distinguished by exporters as they are not deemed semantically different by this specification.

This is very interesting text, and may lead to unexpected behaviors. I may be wrong, but for me personally it is strange that the next two examples are equivalent (and maybe inconsistent across languages):

String val = null
span.SetStringAttribute("my_key", val)

span.SetNullAttribute("my_key")

I am worried that this may lead to strange usages of the API, and may be something we will regret later (maybe I am too conservative). One interesting inconsistency is different between languages, go for example does not have nil strings, so in Java you can set null via SetStringAttribute but cannot do that in go.

[Oberon00]

I think from the discussion on the issue, we'd indeed need a SetNullAttribute and indeed SetAttributer("foo", (String)null), SetAttribute("foo", (Boolean)null), SetNullAttribute("foo") will all do the same. In other languages, SetNullAttribute may be the only way to generate null.

We could, however, explicitly call out that no dedicated way to generate a null value should be provided and nulls values should only be recoded if they naturally occur, and semantic conventions should never prescribe any meaning to null (that is different from "not set"). That way, we would have the null attribute type purely as a debugging aid. Personally, I'd prefer that.

[arminru]

@bogdandrutu Since we don't attribute any semantic difference to a null of type string and a null of no type, I don't see any problem with these two calls being equivalent. Could you please elaborate what bothers you about that?
I also think that it is fine that in Java, where null can occur naturally as the value of a string, it can be recorded using the string attribute API/overload whereas for Go it would have to be recorded using a separate API, if we would desire such an API.

@Oberon00

We could, however, explicitly call out that no dedicated way to generate a null value should be provided and nulls values should only be recoded if they naturally occur, and semantic conventions should never prescribe any meaning to null (that is different from "not set"). That way, we would have the null attribute type purely as a debugging aid. Personally, I'd prefer that.

I'm fine with both but would personally also prefer this over adding an explicit SetNullAttribute API.

If I follow the discussion on #797 correctly, the concerns about the current spec were more about an inconsistent implicit API to delete attributes using null (where available) rather than actually having the desire to express a meaningful null value in languages where this cannot occur naturally but would have to be recorded using an "artificial" SetNullAttribute API. This is certainly the case for @tigrannajaryan as per his last comment there but I'd like @jmacd and @tedsuo to confirm whether they're fine with that as well or if they consider such an API necessary in all languages.

[tigrannajaryan]

If there are concerns about allowing null value I am happy if we go half-way: simply remove the current wording from the spec that says SetAttribute with null-value can be used for deleting attributes. We do not have to also define that null values are valid and how they can be set right now. That addition can be done in the future as long as the "delete" requirement is eliminated from the spec. If desired we can add a note saying that calling SetAttribute with null value is an undefined behavior (but even that probably is not needed).

My main beef is that "delete" requirement.

[arminru]

@tigrannajaryan I considered that as well but thought it would be better to have a defined behavior that is consistent across implementations rather than declaring it as undefined since it might be common enough to happen if one does not explicitly add null checks. While having the delete semantics as error handling is well documented, I agree that it might not be the least surprising behavior.
If we would retreat here and not define null as a valid value, I would rather declare SetAttribute(key, null) as no-op. This would. however, mean that adding null as a valid value later on, if desired, would be a breaking change (since the previous value, if any, would no longer be preserved). This would be the same as with keeping the current delete semantics however.