Skip to content

Commit

Permalink
Add notes on attribute, event, link ordering
Browse files Browse the repository at this point in the history
  • Loading branch information
c24t committed Jul 24, 2019
1 parent 3e6444a commit d2cebe9
Showing 1 changed file with 9 additions and 0 deletions.
9 changes: 9 additions & 0 deletions specification/api-tracing.md
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,10 @@ The Span interface MUST provide:
as arguments. This MAY be called `SetAttribute`. To avoid extra allocations some
implementations may offer a separate API for each of the possible value types.

Attributes SHOULD preserve the order in which they're set. Setting an attribute
with the same key as an existing attribute SHOULD overwrite the existing
attribute's value, but not affect its ordering.

This comment has been minimized.

Copy link
@reyang

reyang Jul 24, 2019

Member

Hmm, this is different from the Python SDK PR, which affects the ordering (by promoting the key to the newest). Which one is the intended behavior?

This comment has been minimized.

Copy link
@c24t

c24t Jul 24, 2019

Author Member

I followed the W3C spec header order for this one but haven't gone back to change BoundedDict yet. I don't have an opinion on the behavior here: I think it makes sense to match W3C, but think it's surprising that keys would be dropped in the order they're added instead of the order they're updated. What do you think is the right behavior?

This comment has been minimized.

Copy link
@reyang

reyang Jul 24, 2019

Member

I don't have personal opinion here :)
W3C spec actually aligns with your PR for BoundedDict (updating a key would result in the key-value-pair to be treated as the latest inserted one).

This comment has been minimized.

Copy link
@c24t

c24t Jul 25, 2019

Author Member

Why isn't the example tracestate at https://github.com/w3c/trace-context/blob/master/spec/20-http_header_format.md#combined-header-value this?

rojo=rojosFirstPosition,congo=congosSecondPosition

This comment has been minimized.

Copy link
@reyang

reyang Jul 25, 2019

Member

W3C Tracestate puts the latest one at the very beginning! :)

https://github.com/w3c/trace-context/blob/master/test/tracecontext/tracestate.py#L44

This comment has been minimized.

Copy link
@reyang

reyang Jul 25, 2019

Member

To summarize:

  1. Updating an existing key-value pair would result in the key being treated as the latest inserted one.
  2. Tracestate and BoundedDict have different order, in Tracestate the latest inserted one shows up in the very beginning of the list, in BoundedDict the latest inserted one show up in the tail.

This comment has been minimized.

Copy link
@c24t

c24t Jul 26, 2019

Author Member

Ah I see, so we just have to reverse this list when we write the header to stay consistent with W3C. So the description and BoundedDict should both be ok as-is.

This comment has been minimized.

Copy link
@reyang

reyang Jul 26, 2019

Member

Yup.

This comment has been minimized.

Copy link
@carlosalberto

carlosalberto Aug 6, 2019

Contributor

I don't think this part has to be aligned with W3C.

This comment has been minimized.

Copy link
@carlosalberto

carlosalberto Aug 6, 2019

Contributor

(Other than that, preserving order sounds fine to me)


Note that the OpenTelemetry project documents certain ["standard
attributes"](../semantic-conventions.md) that have prescribed semantic meanings.

Expand All @@ -345,6 +349,9 @@ by providing an `Event` interface or a concrete `Event` definition and an
`EventFormatter`. If the language supports overloads then this SHOULD be called
`AddEvent` otherwise `AddLazyEvent` may be considered.

Events SHOULD preserve the order in which they're set. This will typically match
the ordering of the events' timestamps.

Note that the OpenTelemetry project documents certain ["standard event names and
keys"](../semantic-conventions.md) which have prescribed semantic meanings.

Expand All @@ -368,6 +375,8 @@ by providing a `Link` interface or a concrete `Link` definition and a
`LinkFormatter`. If the language supports overloads then this MAY be called
`AddLink` otherwise `AddLazyLink` MAY be consider.

Links SHOULD preserve the order in which they're set.

#### Set Status

Sets the [`Status`](#status) of the `Span`. If used, this will override the
Expand Down

0 comments on commit d2cebe9

Please sign in to comment.