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: add startActiveSpan method to Tracer #2221

Merged
merged 27 commits into from
Jun 1, 2021

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented May 21, 2021

Which problem is this PR solving?

Short description of the changes

  • implements startActiveSpan as defined in the trace API

@codecov
Copy link

codecov bot commented May 21, 2021

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%) ⬆️

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

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

naseemkullah added 13 commits May 27, 2021 21:20
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
…textManager

Signed-off-by: naseemkullah <naseem@transit.app>
…tActiveSpan

Signed-off-by: naseemkullah <naseem@transit.app>
Signed-off-by: naseemkullah <naseem@transit.app>
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

@naseemkullah
Copy link
Member Author

Looks great thanks

Np, thanks for the help!

…sed to startActiveSpan

Signed-off-by: naseemkullah <naseem@transit.app>
@dyladan dyladan added the enhancement New feature or request label May 28, 2021
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

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.

[Tracer] implement startActiveSpan()
5 participants