Trace API Final Check

[dyladan]

As we near 1.0 I want to make absolutely 100% sure that we are spec compliant before publishing. To that end, I would ask that as many people as possible, but especially @open-telemetry/javascript-maintainers and @open-telemetry/javascript-approvers take one last look hard at https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md and ensure they don't spot anything required by the spec that we do not have, or forbidden by the spec that we do have. Once we publish we cannot change.

Once you have gone through the spec, please comment here and I will add your name to this list or if you have permission feel free to edit this comment.

Trace Spec Checked by:

Maintainers

• @dyladan
• @vmarchaud
• @obecny

Approvers

• @legendecas
• @markwolff
• @mwear
• @naseemkullah
• @OlivierAlbertini
• @Flarna

Contributors

• ...

Baggage Spec Checked by:

Maintainers

• @dyladan
• @vmarchaud
• @obecny

Approvers

• @legendecas
• @markwolff
• @mwear
• @naseemkullah
• @OlivierAlbertini
• @Flarna

Contributors

• ...

Context Spec Checked by:

Maintainers

• @dyladan
• @vmarchaud
• @obecny

Approvers

• @legendecas
• @markwolff
• @mwear
• @naseemkullah
• @OlivierAlbertini
• @Flarna

Contributors

• ...

[vmarchaud]

I think we need to wait for #1764, #1749, #1759 and find a settlement for #1750 before. They all modify the api package and involves methods that user will use for tracing.

[dyladan]

@vmarchaud I am really just asking for people to take a look through the spec for things we've missed. Since those issues are tracked already and we're aware of them, they're ok. But for instance I had not realized the methods removed in #1764 were removed from the spec and I want to make sure we're not missing more things like that.

[vmarchaud]

@dyladan Oh i see, then i'll let my comment as-is since others might find it useful to know that PRs are already opened for some changes in the spec

[Flarna]

There is an inconsistency between tracer.startSpan() and span.end() regarding timestamps passed by user:

• SpanOptions #startTime is of type number
• span.end() accepts TimeInput which is HrTime | number | Date

[srikanthccv]

Shouldn't TraceState implementation (https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-core/src/trace/TraceState.ts) be part of API according to spec?

[obecny]

Shouldn't TraceState implementation (https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-core/src/trace/TraceState.ts) be part of API according to spec?

It looks like you are right, unless I'm missing something else, @dyladan ?

[Flarna]

The span.setAttribute() API could be improved. It tells null and undefined are not allowed for value but type definition has value optional.

opentelemetry-js/packages/opentelemetry-api/src/trace/span.ts

Lines 44 to 53 in 1a3f3f8

	/**
	* Sets an attribute to the span.
	*
	* Sets a single Attribute with the key and value passed as arguments.
	*
	* @param key the key for this attribute.
	* @param value the value for this attribute. Setting a value null or
	* undefined is invalid and will result in undefined behavior.
	*/
	setAttribute(key: string, value?: AttributeValue): this;

[dyladan]

Shouldn't TraceState implementation (https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-core/src/trace/TraceState.ts) be part of API according to spec?

It looks like you are right, unless I'm missing something else, @dyladan ?

The actual implementation? The interface is already in.