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

Fix NoopTracer with no parent #1605

Closed
wants to merge 1 commit into from

Conversation

pauldraper
Copy link
Contributor

Which problem is this PR solving?

#1603

Short description of the changes

Check for null parent

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 19, 2020

CLA Check
The committers are authorized under a signed CLA.

@pauldraper
Copy link
Contributor Author

I signed the CLA.

@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1605   +/-   ##
=======================================
  Coverage   91.18%   91.18%           
=======================================
  Files         164      164           
  Lines        5024     5025    +1     
  Branches     1026     1027    +1     
=======================================
+ Hits         4581     4582    +1     
  Misses        443      443           
Impacted Files Coverage Δ
packages/opentelemetry-api/src/trace/NoopTracer.ts 100.00% <100.00%> (ø)

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

could you add a test so we doesnt introduce a regression in the future ?

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 agree a test for this would be welcome

@dyladan dyladan added the bug Something isn't working label Nov 12, 2020
@dyladan
Copy link
Member

dyladan commented Nov 18, 2020

@pauldraper can you please add the test as @vmarchaud suggested?

@Flarna
Copy link
Member

Flarna commented Nov 19, 2020

This seems to be outdated as the SpanOption {parent: null} has been replaced by {root: true}.

@dyladan
Copy link
Member

dyladan commented Nov 19, 2020

isSpanContext is still called on the spancontext extracted from context to ensure a valid context object was provided.

@Flarna
Copy link
Member

Flarna commented Nov 19, 2020

Yes, but it will be never null - at least according to typescript.

@dyladan
Copy link
Member

dyladan commented Dec 16, 2020

@Flarna is right closing

@dyladan dyladan closed this Dec 16, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
* chore: fix issue with nyc crashing

* chore: revert hoist change, make @babel/core nohoist

* chore: update @babel/core to test Garbage Collector issue

* chore: bump typescripts to 4.4.4 in some package

* Revert "chore: bump typescripts to 4.4.4 in some package"

This reverts commit 5bf1bfbe433bf614787c0ccca79a40aa644c86b3.

* Revert "Revert "chore: bump typescripts to 4.4.4 in some package""

This reverts commit 2877b7e500b629216a66f4b9300e82e3d452ab4f.

* Fix cancelled issue

---------

Co-authored-by: Haddas Bronfman <85441461+haddasbronfman@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants