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

chore: use span interface from API #2075

Closed
wants to merge 3 commits into from

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Apr 5, 2021

In #2065 an issue was raised that compilation can fail if multiple versions of tracing are installed because the SpanProcessor interface refers to the concrete Span class instead of the interface from the API.

Using the interface makes this module more backwards compatible.

@codecov
Copy link

codecov bot commented Apr 5, 2021

Codecov Report

Merging #2075 (aeed39c) into main (3d4d8b5) will not change coverage.
The diff coverage is 100.00%.

❗ Current head aeed39c differs from pull request most recent head ce482c4. Consider uploading reports for the commit ce482c4 to get more accurate results

@@           Coverage Diff           @@
##             main    #2075   +/-   ##
=======================================
  Coverage   93.03%   93.03%           
=======================================
  Files         154      154           
  Lines        5976     5976           
  Branches     1246     1246           
=======================================
  Hits         5560     5560           
  Misses        416      416           
Impacted Files Coverage Δ
...es/opentelemetry-tracing/src/MultiSpanProcessor.ts 100.00% <ø> (ø)
...ges/opentelemetry-tracing/src/NoopSpanProcessor.ts 42.85% <ø> (ø)
...telemetry-tracing/src/export/BatchSpanProcessor.ts 91.20% <100.00%> (ø)
...elemetry-tracing/src/export/SimpleSpanProcessor.ts 82.75% <100.00%> (ø)

@Flarna
Copy link
Member

Flarna commented Apr 6, 2021

This seems to be inconsistent now.

  • interface SpanProcessor#onStart gets a api.Span now
  • implementations like BatchSpanProcessor and MultiSpanProcessor still use the class Span
  • interface SpanProcessor#onEnd gets a ReadableSpan (class, not interface)

Please note the subtle difference in the APIs, e.g. there is ReadableSpan#ended or ReadableSpan#resource which are also available in class Span but not in api.Span.

@dyladan
Copy link
Member Author

dyladan commented Apr 7, 2021

  • interface SpanProcessor#onStart gets a api.Span now

yes

  • implementations like BatchSpanProcessor and MultiSpanProcessor still use the class Span

Class span implements the Span interface expected by the SpanProcessor interface so this is OK. The SpanProcessor will only ever receive spans created by ourselves so we know it will always receive this concrete implementation. In any case, I've changed it to use the interface instead.

  • interface SpanProcessor#onEnd gets a ReadableSpan (class, not interface)

It is an interface https://github.com/dyladan/opentelemetry-js/blob/span-processor-interface/packages/opentelemetry-tracing/src/export/ReadableSpan.ts#L29

Please note the subtle difference in the APIs, e.g. there is ReadableSpan#ended or ReadableSpan#resource which are also available in class Span but not in api.Span.

As long as an implementation which satisfies the interface is used, it should work.

@Flarna
Copy link
Member

Flarna commented Apr 7, 2021

The SpanProcessor will only ever receive spans created by ourselves so we know it will always receive this concrete implementation. In any case, I've changed it to use the interface instead.

Well that's true but if you want to access any detail from class Span it requires a cast now. Typescript will no longer complain if e.g. a NoopSpan is given to onStart().

The existing SpanProcessors in this repo have no need for onStart() but someone might create an improved processor where onStart() has a real implementation which wants to use the rich data from class Span.

I'm not sure if we really want to remove this functionality to avoid issues caused by a mixed version setup.

@dyladan
Copy link
Member Author

dyladan commented Apr 7, 2021

This might not be needed once #2073 is merged anyway

@obecny
Copy link
Member

obecny commented Apr 12, 2021

This might not be needed once #2073 is merged anyway

it was merged, does it mean it is not needed now or ?

@dyladan
Copy link
Member Author

dyladan commented Apr 12, 2021

Probably. I'll close for now but not delete the branch until we release with pinned versions and ensure the problem is actually fixed.

@dyladan dyladan closed this Apr 12, 2021
@dyladan dyladan deleted the span-processor-interface branch August 26, 2021 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants