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

Move SpanContext isValid to the API #1447

Merged
merged 24 commits into from
Aug 31, 2020

Conversation

srjames90
Copy link
Contributor

@srjames90 srjames90 commented Aug 19, 2020

Which problem is this PR solving?

Fixes #1414

Short description of the changes

Changes SpanContext interface to a class and adds an isValid method to it. This also updates every reference to SpanContext to instantiate the spanContext using new instead of an object literal matching the interface.

Started as draft in case this isn't what we had in mind since it ends up changing so many files. I had to go through so many files because the interfaces were being implemented with object literals.

@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #1447 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1447   +/-   ##
=======================================
  Coverage   93.99%   93.99%           
=======================================
  Files         153      153           
  Lines        4663     4666    +3     
  Branches      962      963    +1     
=======================================
+ Hits         4383     4386    +3     
  Misses        280      280           
Impacted Files Coverage Δ
packages/opentelemetry-api/src/api/trace.ts 92.85% <100.00%> (+0.54%) ⬆️
packages/opentelemetry-api/src/trace/NoopSpan.ts 100.00% <100.00%> (ø)
...s/opentelemetry-api/src/trace/spancontext-utils.ts 100.00% <100.00%> (ø)
...es/opentelemetry-core/src/trace/NoRecordingSpan.ts 100.00% <100.00%> (ø)
packages/opentelemetry-tracing/src/Tracer.ts 100.00% <100.00%> (ø)

@srjames90 srjames90 requested a review from dyladan August 24, 2020 21:27
@dyladan
Copy link
Member

dyladan commented Aug 25, 2020

The spec suggests adding a helper function. open-telemetry/opentelemetry-specification#771 (comment)

@srjames90 can you implement api.trace.isSpanContextValid(spanContext: SpanContext): boolean function?

Making changes to the ~30 callers is fine.

@srjames90
Copy link
Contributor Author

The spec suggests adding a helper function. open-telemetry/opentelemetry-specification#771 (comment)

@srjames90 can you implement api.trace.isSpanContextValid(spanContext: SpanContext): boolean function?

Making changes to the ~30 callers is fine.

Cool, sounds good. Sounds like the current changes are fine, and just have to rename the function. Thanks, will do it now.

@srjames90 srjames90 requested a review from dyladan August 25, 2020 22:12
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.

I have to block because you have broken the spec compliance of the B3 propagator. Everything else looks good.

@dyladan
Copy link
Member

dyladan commented Aug 26, 2020

Looks like @srjames90 put the function at api.isSpanContextValid instead of api.trace.isSpanContextValid. @obecny think we should have him move it or keep it there?

@obecny
Copy link
Member

obecny commented Aug 26, 2020

Looks like @srjames90 put the function at api.isSpanContextValid instead of api.trace.isSpanContextValid. @obecny think we should have him move it or keep it there?

@srjames90 pls move the function to api.trace.isSpanContextValid

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM

@dyladan dyladan changed the title Span context is valid Move SpanContext isValid to the API Aug 31, 2020
@dyladan dyladan added API enhancement New feature or request labels Aug 31, 2020
@obecny obecny merged commit b88b95b into open-telemetry:master Aug 31, 2020
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* refactor: make spanContext into class with isValid

* refactor: use class instantiation for spanContext

* refactor: moves test and fix tests

* fix: add check for isValid

* revert: the changes to spanContext

* refactor: move spancontext-utils to api package

* fix: lint and invalid psan id and invalid trace id

* fix: make isValid more robust

* refactor: rename isValid to isSpanContextValid

* refactor: update regex and checks

* fix: export isSpanContextValid from api.trace

* fix: run npm run lint:fix

* fix: run lint fix in api

* refactor: prevent function overhead

* fix: lint

* fix: remove unused import

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* refactor: make spanContext into class with isValid

* refactor: use class instantiation for spanContext

* refactor: moves test and fix tests

* fix: add check for isValid

* revert: the changes to spanContext

* refactor: move spancontext-utils to api package

* fix: lint and invalid psan id and invalid trace id

* fix: make isValid more robust

* refactor: rename isValid to isSpanContextValid

* refactor: update regex and checks

* fix: export isSpanContextValid from api.trace

* fix: run npm run lint:fix

* fix: run lint fix in api

* refactor: prevent function overhead

* fix: lint

* fix: remove unused import

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API SpanContext missing isValid API
7 participants