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

feat: introduce ended property on Span #625

Merged
merged 10 commits into from
Mar 10, 2020

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Dec 16, 2019

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
Copy link

codecov-io commented Dec 16, 2019

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
Copy link
Member

dyladan commented Dec 16, 2019

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
Copy link
Member Author

Flarna commented Dec 16, 2019

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
Copy link
Member

dyladan commented Dec 16, 2019

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
Copy link
Member

dyladan commented Dec 16, 2019

@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
Copy link
Member

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 dyladan added enhancement New feature or request waiting-for-spec labels Dec 16, 2019
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

Flarna commented Dec 18, 2019

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
Copy link
Member

dyladan commented Dec 18, 2019

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

Is there a spec issue created for that?

@Flarna
Copy link
Member Author

Flarna commented Dec 18, 2019

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

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
Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Flarna
Copy link
Member Author

Flarna commented Feb 24, 2020

@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
Copy link
Member

dyladan commented Mar 4, 2020

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
Copy link
Member Author

Flarna commented Mar 5, 2020

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

@dyladan
Copy link
Member

dyladan commented Mar 5, 2020

yes

@dyladan
Copy link
Member

dyladan commented Mar 5, 2020

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

@Flarna
Copy link
Member Author

Flarna commented Mar 5, 2020

Merged master and reverted API changes.

@mayurkale22
Copy link
Member

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
Copy link
Member Author

Flarna commented Mar 6, 2020

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...

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mayurkale22
Copy link
Member

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

@dyladan dyladan merged commit 7498ae6 into open-telemetry:master Mar 10, 2020
@Flarna Flarna deleted the span-ended-public branch March 10, 2020 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting-for-spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants