feat: introduce ended property on Span

[Flarna]

Which problem is this PR solving?

Allows a SpanProcessor and SpanExporter to check if a Span is ended or not.

Short description of the changes

Add a getter to Span and ReadableSpan to allow checking if the span is ended without using a private field or rely on internals like span.duration[0] < 0.

Refs: open-telemetry/opentelemetry-java#693
Refs: open-telemetry/opentelemetry-java#697

[codecov-io]

Codecov Report

Merging #625 into master will increase coverage by 2.66%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #625      +/-   ##
==========================================
+ Coverage   93.35%   96.01%   +2.66%     
==========================================
  Files         202      203       +1     
  Lines        7869     8718     +849     
  Branches      708      771      +63     
==========================================
+ Hits         7346     8371    +1025     
+ Misses        523      347     -176

Impacted Files	Coverage Δ
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts	100% <ø> (ø)
...entelemetry-exporter-jaeger/test/transform.test.ts	100% <ø> (ø)	⬆️
.../opentelemetry-exporter-jaeger/test/jaeger.test.ts	100% <ø> (ø)	⬆️
...-exporter-stackdriver-trace/test/transform.test.ts	100% <ø> (ø)	⬆️
...y-exporter-stackdriver-trace/test/exporter.test.ts	100% <ø> (ø)	⬆️
packages/opentelemetry-tracing/src/Span.ts	100% <100%> (ø)	⬆️
packages/opentelemetry-tracing/test/Span.test.ts	100% <100%> (ø)	⬆️
...ckages/opentelemetry-core/src/common/NoopLogger.ts	60% <0%> (-20%)	⬇️
...es/opentelemetry-node/src/instrumentation/utils.ts	90.47% <0%> (-9.53%)	⬇️
...metry-core/src/trace/instrumentation/BasePlugin.ts	81.57% <0%> (-5.27%)	⬇️
... and 113 more

[dyladan]

Is that something we should have the spec add? As far as I know, the span processors are supposed to rely only on the public Span API type, so you will have to cast it to the sdk version of the span to read isEnded from it anyways.

I’m generally confused by the existence of the ReadableSpan type. The spec seems to assume that the exporters will be able to access all of the properties of the span, but it explicitly states that the span should not export properties other than its span context

I can’t find any mention of ReadableSpan in the spec. Our span is not spec compliant in this way as it also exports all properties of ReadableSpan which is actually explicitly disallowed in the spec.
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#span says: “To prevent misuse, implementations SHOULD NOT provide access to a Span’s attributes besides its SpanContext”

I don’t know how other SIGs reconcile this, but my gut feeling is that they do the same thing we do, which is exporters/processors just assume the sdk span type and not the api type

In order to make our Span spec compliant, we will have to remove all ReadableSpan properties from the Span and make them private. We will then need a way to convert it into a ReadableSpan object for export.

[Flarna]

Good catch!

There seems to be an inconsistency in existing code of @opentelemetry/tracing. SimpleSpanProcessor uses import { Span } from '../Span' - the implementation I modified.
But interface SpanProcessor uses import { Span } from '@opentelemetry/types' which I haven't touched - and touching it would be most likely a spec topic.
Span implementation is exported from @opentelemetry/tracing so it seems to be fine to use it in exporters.
But I think it needs some sort of clarifications if this is really intended. Personally I think isEnded is not needed in Api layer. But if it is there it should not harm as it may ease implementations in plugins which have several possible end events (e.g. an optional error event and a close event emitted).

I think the wording in spec is only regarding API not processor/exporter interface. If an exporter gets only the SpanContext vital information like start/end time and attributes would be missing.

[dyladan]

As far as I know, and we just went through this with the xhr plugin, the plugins/exporters/spanprocessors should use the public api only because a third party tracer could be used which has a different implementation of Span.

[dyladan]

@Flarna according to @mayurkale22 the export pipeline is tightly coupled with sdk, so it should be safe to use properties that are on our Span, but not in the public interface. If a third party sdk is plugged in, they will have to provide their own export pipeline.

edit: @mayurkale22 please confirm if you can

[mayurkale22]

AFAIK our SDK implementation is tightly coupled with export pipeline. If you're interested writing your own sdk, you get complete freedom on export pipeline.

Reg. this PR I would wait for the specs issue to get approved and merged: open-telemetry/opentelemetry-specification#373

[dyladan]

Giving my approval to this since in the current spec the export pipeline is tightly coupled with the SDK.

In the future I would like to propose a more clear separation of the SDK and the export pipeline so that it is easier to plug third party components without requiring them to replace the full SDK.

[Flarna]

Looking closer into this I noticed that it's needed to update also types.Span.

According to spec a SpanProcessor should get only a ReadableSpan but currently it gets a class Span.

I tried to correct this but it turns out that we can create a ReadableSpan only from class Span.

The label wait-for-spec is already set so that is fine.

[dyladan]

The label wait-for-spec is already set so that is fine.

Is there a spec issue created for that?

[Flarna]

I expect open-telemetry/opentelemetry-specification#373 covers this.

[OlivierAlbertini]

LGTM

[Flarna]

@dyladan @mayurkale22 Is there any chance to move this forward?
I'm not that clear if we really have to wait on spec or not.
Or if the current mix between Api.Span, Core.Span and ReadableSpan and where they are used something we have to change?

[dyladan]

This is all within the export pipeline which I have been informed by spec is an SDK-only concern so we can use non-API methods and properties within the export pipeline. This means we can just add a property/method to our span.

One side-effect of this is that it will tightly couple the export pipeline to our span implementation, which we have avoided so far. This may make it more difficult for a third party SDK implementation to take advantage of our export pipeline, but does not explicitly violate spec.

I think given all of that, we can move forward with this.

[Flarna]

@dyladan So in short I should remove the changes in API (span.ts and NoopSpan.ts) but keep the rest as it is?

[dyladan]

yes

[dyladan]

@mayurkale22 let me know if that is an issue for you

[Flarna]

Merged master and reverted API changes.

[mayurkale22]

Add a getter to Span and ReadableSpan to allow checking if the span is ended without using a private field or rely on internals like span.duration[0] < 0

Are you planning to update this in separate PR?

[Flarna]

Are you planning to update this in separate PR?

As @dyladan requested to remove the changes from API as they are not part of the spec it's not possible to use this within plugins. If they have the need to detect if a span is already ended they have to store it somewhere else. Unfortunately this is what the spec requires...

[mayurkale22]

lgtm

[mayurkale22]

Looks like this is good to merge, just wanted to check with @dyladan on waiting-for-spec label.