Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give a definition for null-valued attributes/labels/resources #503

Closed
jmacd opened this issue Mar 4, 2020 · 6 comments
Closed

Give a definition for null-valued attributes/labels/resources #503

jmacd opened this issue Mar 4, 2020 · 6 comments
Labels
priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory

Comments

@jmacd
Copy link
Contributor

jmacd commented Mar 4, 2020

In #459 we declared that empty-string values are meaningful. I agree with this position and recommend the same treatment for the metrics API. However, the same change introduces language about null-valued span attributes, stating that they are effectively skipped and/or indicate deletion of an attribute.

Attribute values of `null` are considered to be not set and get discarded as if that `SetAttribute` call had never been made.
As an exception to this, if overwriting of values is supported, this results in clearing the previous value and dropping the attribute key from the set of attributes.

This topic came up in the metrics API because many exporters will treat an unspecified value as an empty string in the exposition format. The default Metrics SDK specification will have to say something about this particular point. See the related discussion in #345.

As for null, this is something of a language-specific term. For example, there is no way to construct a null-valued attribute in the current Go API, because there simply is not a method to create a null-valued KeyValue. The Go API uses an "INVALID" type to indicate when a variable has not been initialized, and we could possibly treat the uninitialized state as null. However, I would call the uninitialized state an error, and would prefer the API tell me that I have made an error rather than delete an attribute because I "set" the uninitialized value.

As for the value of null, it is a value. It is a value in the language and it is a value in JSON, and I would argue that we should have an explicit null-valued attribute, metric label, and resource. In export formats that preserve value type, this would have its own distinct type in the protocol (i.e., in JSON "null" vs null). In export formats that require a string representation, the representation is "null", not the empty string, because these are meaningfully different values.

My request is that we declare a meaningful null value. The call to SetAttribute is not skipped, but a null-value is recorded.

If we intend to support a Span API for deleting a current annotation (as discussed when setting null-valued attributes in #459)--and I do not think we should--then we should have an explicit API for deleting span attributes (e.g., DeleteAttribute). However, I'll note that this topic was briefly discussed because OpenCensus and OpenTracing have a notion of "overwrite" but not of "delete" for attributes: open-telemetry/opentelemetry-go#53 (comment)

@jmacd
Copy link
Contributor Author

jmacd commented Mar 4, 2020

A closely related point on #459 is about this language specifying valid keys:

MUST be a non-null and non-empty string.

We also have a guideline not to panic, so what should be done if the user supplies a key that is an empty string? I do not believe the right answer is to introduce error checking (i.e., require you to check errors when constructing keys). I agree, however, with the requirement, so I would say something like implementations SHOULD use a name "empty_at_file:line" when the empty-string is given as a key name, or "null" (or something similar), when a null-valued key is provided.

(Note that in Go, the key is a string type, so it's impossible to pass a null-valued key.)

@arminru
Copy link
Member

arminru commented Mar 5, 2020

If there is no way of adding a null value in Go anyway, then I'd say this is fine and does not warrant any spec change since the cited paragraph just won't apply to Go in this place.
It sounds like the invalid/uninitialized type in Go is more explicit and clearly defined than the null value encountered in other languages, where many APIs will just return null for values that were not found. See the discussion on open-telemetry/opentelemetry-java#765, for example. Therefore I'd say it's fine for Go to deviate from the spec for this explicit invalid/uninitialized type and don't clear existing values if this is attempted to be set.

I'm open to discuss dropping the null-clears-previous-value behavior in favor of an explicit DeleteAttribute method. If we disallow clearing previously set attributes, I think this would just lead to users using workarounds like set("") or set("<removed>") in case they want to drop an attribute which feels messy to me. This discussion should, however, take place on a separate issue.

While it would be required to be able to export a meaningful null value in proto3, for example, replacing null with a string valued "null" does not feel desirable to me. This would require quite some special handling in back ends that won't expect a certain string to be equivalent to a value that's not set.

If we cannot agree on keeping the current semantics, I would at least like to leave it up to language implementations to decide if null should be considered meaningful or rather unspecified/not-set instead of recording and sending each and every null value.

@MikeGoldsmith
Copy link
Member

Rust also does not support null and instead provides Option<T> and uses the terms Some and None to determine if a value has been initialised or not.

I don't think tying to add the concept of null into languages that does not have it is not the right path.

I think simply adding an attribute indicate we want it to be used, and not ignored. What if we replaced null attribute values with an empty string "" if passed to SetAttribute? We then know it's a valid, cross-language value and won't be omitted.

@pauldraper
Copy link

Two choices

  1. Have SetAttribute() accept an optional value (nullable, Maybe...whatever the language uses for optional values). If the value is none, it is removed.

  2. Or, add a RemoveAttribute() function.

@Oberon00
Copy link
Member

I prefer doing neither of the above. As I see it, only if we cannot disallow null at compile time, do we need to define some sensible behavior for it. If at all possible, there should be no null/remove/... at all.

@carlosalberto carlosalberto added spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory spec:metrics Related to the specification/metrics directory release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Jun 26, 2020
@mtwo mtwo added this to Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. in GA Spec Burndown Jul 6, 2020
@andrewhsu andrewhsu added the priority:p2 Medium priority level label Jul 17, 2020
@jmacd
Copy link
Contributor Author

jmacd commented Jul 21, 2020

This is unpopular, I'll close it.

@jmacd jmacd closed this as completed Jul 21, 2020
GA Spec Burndown automation moved this from Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. to Required for GA, done Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory
Projects
No open projects
GA Spec Burndown
  
Required/Allowed for GA, resolved.
Development

No branches or pull requests

7 participants