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.

Sign up for GitHub

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

Jump to bottom

feat: add startActiveSpan method to Tracer #2221

Merged
merged 27 commits into from Jun 1, 2021
Merged

feat: add startActiveSpan method to Tracer #2221

merged 27 commits into from Jun 1, 2021

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented May 21, 2021 •
edited

Which problem is this PR solving?

Short description of the changes

  • implements startActiveSpan as defined in the trace API

@naseemkullah naseemkullah requested a review from hekike May 21, 2021 18:45
@codecov
Copy link

codecov bot commented May 21, 2021 •
edited

Codecov Report

Merging #2221 (d5bab9b) into main (d551781) will increase coverage by 0.41%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main    #2221      +/-   ##
==========================================
+ Coverage   92.29%   92.71%   +0.41%     
==========================================
  Files         144      144              
  Lines        5168     5160       -8     
  Branches     1064     1056       -8     
==========================================
+ Hits         4770     4784      +14     
+ Misses        398      376      -22
Impacted Files Coverage Δ
packages/opentelemetry-tracing/src/Tracer.ts 98.38% <93.33%> (+31.24%) ⬆️

dyladan
dyladan reviewed May 21, 2021
View reviewed changes
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.

This should fix your overload issues i think

packages/opentelemetry-tracing/src/Tracer.ts Outdated Show resolved Hide resolved
packages/opentelemetry-tracing/src/Tracer.ts Outdated Show resolved Hide resolved
@naseemkullah naseemkullah force-pushed the start-active-span branch 3 times, most recently from 869a3d2 to 47fd5f6 Compare May 21, 2021 19:38
vmarchaud
vmarchaud reviewed May 21, 2021
View reviewed changes
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.

i think it makes sense to add test in both the node sdk (which bundle the async hooks context manager) and the web (with the zone context manager)

obecny
obecny reviewed May 21, 2021
View reviewed changes
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

I did so far all necessary refactoring for the core repo with regards to api 0.20 just waiting for it to be released. I took exactly the same implementation that exists already in api for this function. Maybe I'm missing something but why it cannot be the same ?, your ts doc is much cooler than mine though :D. So curious why it needs to be different ?

@naseemkullah
Copy link
Member Author

I did so far all necessary refactoring for the core repo with regards to api 0.20 just waiting for it to be released. I took exactly the same implementation that exists already in api for this function. Maybe I'm missing something but why it cannot be the same ?, your ts doc is much cooler than mine though :D. So curious why it needs to be different ?

Oh good point, I guess you. mean the implementation in noop tracer? Thats what I copy pasted originally here but @dyladan pointed out the preference for function overloads which I agree with, just could not get right in the api PR for the noop tracer implementation.

As for the ts doc its the one I put in the tracer interface :)

With that said, I think this implementation is better and can make a PR to api repo to overwrite the noop tracer implementation, wdyt?

@naseemkullah
Copy link
Member Author

open-telemetry/opentelemetry-js-api#81 created to make implementation same

@obecny obecny mentioned this pull request May 24, 2021
Merged
naseemkullah added 14 commits May 27, 2021 21:20
test: startActiveSpan ensure that ctx sent is used
ee7a305 
also add commented out span equality assertions to be uncommented before
merging

Signed-off-by: naseemkullah <naseem@transit.app>
chore: organize imports
3ab1fe4 
Signed-off-by: naseemkullah <naseem@transit.app>
chore: clarify todo
02b5dfd 
Signed-off-by: naseemkullah <naseem@transit.app>
test: startActiveSpan in NodeTracer
a1e5bdb 
Signed-off-by: naseemkullah <naseem@transit.app>
chore: put active span tests in own describe block
0a00ddf 
Signed-off-by: naseemkullah <naseem@transit.app>
test: startActiveSpan in WebTracer
e6eb4f7 
Signed-off-by: naseemkullah <naseem@transit.app>
chore: remove Tracer specific tests from Node and Web Tracers
41ff0b1 
Signed-off-by: naseemkullah <naseem@transit.app>
fix: conflicts
374c5ea 
Signed-off-by: naseemkullah <naseem@transit.app>
chore: remove git clean form reset script
63ec701 
Signed-off-by: naseemkullah <naseem@transit.app>
fix: conflicts
0afe1ee 
Signed-off-by: naseemkullah <naseem@transit.app>
fix: use span.spanContext()
5532776 
Signed-off-by: naseemkullah <naseem@transit.app>
fix: adhere to breaking changes in new api
4219630 
Signed-off-by: naseemkullah <naseem@transit.app>
chore: only import api once
0335473 
Signed-off-by: naseemkullah <naseem@transit.app>
test: assert active spans are indeed active with help of TestStackCon…
1944b4e 
…textManager

Signed-off-by: naseemkullah <naseem@transit.app>
test: spy startSpan to ensure it is called with opts passed into star…
ec33257 
…tActiveSpan

Signed-off-by: naseemkullah <naseem@transit.app>
fix: uncomment sinon spy
c395c81 
Signed-off-by: naseemkullah <naseem@transit.app>
dyladan
dyladan approved these changes May 28, 2021
View reviewed changes
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.

Looks great thanks

packages/opentelemetry-tracing/test/Tracer.test.ts Show resolved Hide resolved
@naseemkullah
Copy link
Member Author

Looks great thanks

Np, thanks for the help!

test: assert startSpan spy called with name when only name and fn pas…
4eb9d6c 
…sed to startActiveSpan

Signed-off-by: naseemkullah <naseem@transit.app>
@dyladan dyladan added the enhancement New feature or request label May 28, 2021
obecny
obecny approved these changes May 28, 2021
View reviewed changes
Copy link
Member

@obecny obecny 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 merged commit aea89cf into open-telemetry:main Jun 1, 2021
@naseemkullah naseemkullah deleted the start-active-span branch June 1, 2021 20:26
@dyladan dyladan mentioned this pull request Jun 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obecny obecny obecny approved these changes

@dyladan dyladan dyladan approved these changes

@hekike hekike Awaiting requested review from hekike

@johnbley johnbley Awaiting requested review from johnbley

@legendecas legendecas Awaiting requested review from legendecas

@markwolff markwolff Awaiting requested review from markwolff

@mayurkale22 mayurkale22 Awaiting requested review from mayurkale22

@mwear mwear Awaiting requested review from mwear

@OlivierAlbertini OlivierAlbertini Awaiting requested review from OlivierAlbertini

@vmarchaud vmarchaud Awaiting requested review from vmarchaud

@Flarna Flarna Awaiting requested review from Flarna

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Tracer] implement startActiveSpan()
5 participants
@naseemkullah @obecny @dyladan @vmarchaud @Flarna