Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

feat: add tracer.startActiveSpan() #54

Merged
merged 1 commit into from
May 17, 2021
Merged

feat: add tracer.startActiveSpan() #54

merged 1 commit into from
May 17, 2021

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented May 1, 2021 •
edited
Loading

adds helper function tracer.startActiveSpan() which creates a new span and activates it in the current Context

naseemkullah
naseemkullah commented May 1, 2021
View reviewed changes
src/trace/tracer.ts Outdated Show resolved Hide resolved
@vmarchaud
Copy link
Member

Since this isn't in the spec and we have the problem for the suppress instrumentation system, can we even merge this in the API ?

@naseemkullah naseemkullah force-pushed the tracer-start-active-span branch 2 times, most recently from 2775dd7 to ba070ae Compare May 1, 2021 21:05
@naseemkullah
Copy link
Member Author

Since this isn't in the spec and we have the problem for the suppress instrumentation system, can we even merge this in the API

According to spec this functionality may be offered by a separate operation, would this not be it? #14 (comment)

I'm ignorant to the suppress instrumentation problem though.

@vmarchaud
Copy link
Member

I'm ignorant to the suppress instrumentation problem though.

Its our system to avoid exporting loop to "stop" instrumentation from generating spans which is an additional "operation/API" too but we've been asked to either add it to the spec or package it as a seperate npm package.

According to spec this functionality may be offered by a separate operation, would this not be it? #14 (comment)

Looks like the spec acknowledge it indeed so this should be fine to add

vmarchaud
vmarchaud reviewed May 2, 2021
View reviewed changes
src/trace/tracer.ts Outdated Show resolved Hide resolved
@naseemkullah naseemkullah force-pushed the tracer-start-active-span branch 3 times, most recently from 8b4e002 to d43f446 Compare May 2, 2021 17:48
@Flarna
Copy link
Member

Flarna commented May 2, 2021

we have the problem for the suppress instrumentation system

What is the problem regarding suppress instrumentation here? Isn't this API is just a helper around exiting APIs?

@codecov
Copy link

codecov bot commented May 3, 2021 •
edited
Loading

Codecov Report

Merging #54 (36a54da) into main (8435e0a) will decrease coverage by 4.34%.
The diff coverage is 13.79%.

❗ Current head 36a54da differs from pull request most recent head 81fd6a9. Consider uploading reports for the commit 81fd6a9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #54      +/-   ##
==========================================
- Coverage   94.66%   90.31%   -4.35%     
==========================================
  Files          40       40              
  Lines         506      537      +31     
  Branches       80       92      +12     
==========================================
+ Hits          479      485       +6     
- Misses         27       52      +25
Impacted Files Coverage Δ
src/trace/NoopTracer.ts 48.93% <11.53%> (-46.52%) ⬇️
src/trace/ProxyTracer.ts 84.21% <33.33%> (-9.54%) ⬇️
src/api/propagation.ts 100.00% <0.00%> (ø)
src/trace/context-utils.ts 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8435e0a...81fd6a9. Read the comment docs.

@naseemkullah naseemkullah force-pushed the tracer-start-active-span branch from 22116c0 to 81151c8 Compare May 3, 2021 01:32
@dyladan
Copy link
Member

dyladan commented May 3, 2021

we have the problem for the suppress instrumentation system

What is the problem regarding suppress instrumentation here? Isn't this API is just a helper around exiting APIs?

No there is no technical problem. I think @vmarchaud was using it as an example of a feature that we implemented without spec and got pushback from the TC.

@Flarna Flarna mentioned this pull request May 3, 2021
Closed
@dyladan
Copy link
Member

dyladan commented May 3, 2021

Why is this WIP?

@naseemkullah
Copy link
Member Author

Why is this WIP?

It is not ready to be merged in its current state, and am still requesting comments/direction, e.g.: #54 (comment)

dyladan
dyladan reviewed May 3, 2021
View reviewed changes
src/trace/tracer.ts Show resolved Hide resolved
src/trace/tracer.ts Outdated Show resolved Hide resolved
@naseemkullah naseemkullah force-pushed the tracer-start-active-span branch from 8fcd56f to 997ccaa Compare May 4, 2021 10:21
@naseemkullah naseemkullah requested review from dyladan and vmarchaud May 4, 2021 10:23
@naseemkullah naseemkullah marked this pull request as ready for review May 4, 2021 10:23
@naseemkullah naseemkullah force-pushed the tracer-start-active-span branch from 3a119c5 to 56f11e2 Compare May 12, 2021 17:51
@naseemkullah naseemkullah marked this pull request as ready for review May 12, 2021 18:06
@naseemkullah naseemkullah force-pushed the tracer-start-active-span branch from b77b2e2 to 8eb602d Compare May 13, 2021 01:38
vmarchaud
vmarchaud approved these changes May 13, 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.

lgtm, tests are failing though

@vmarchaud vmarchaud requested review from obecny, dyladan and Flarna May 13, 2021 09:53
Flarna
Flarna approved these changes May 13, 2021
View reviewed changes
Copy link
Member

@Flarna Flarna left a comment

Choose a reason for hiding this comment

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

A test for NoopTracer to verify that fn is actually called correct and return value is passed through would be nice.

src/trace/tracer.ts Outdated Show resolved Hide resolved
src/trace/NoopTracer.ts Outdated Show resolved Hide resolved
test/proxy-implementations/proxy-tracer.test.ts Outdated Show resolved Hide resolved
obecny
obecny approved these changes May 13, 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

@naseemkullah naseemkullah force-pushed the tracer-start-active-span branch 2 times, most recently from 58cf7eb to 18b2ae3 Compare May 14, 2021 12:42
dyladan
dyladan reviewed May 14, 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.

Hopefully the comment I left can help you with your sinon stub typing

src/trace/tracer.ts Outdated Show resolved Hide resolved
src/trace/ProxyTracer.ts Outdated Show resolved Hide resolved
@naseemkullah
Copy link
Member Author

Hopefully the comment I left can help you with your sinon stub typing

Thanks! 🙏

@naseemkullah
Copy link
Member Author

Hopefully the comment I left can help you with your sinon stub typing

In the end it was still broken, sinon types + overloads don't play nicely as per DefinitelyTyped/DefinitelyTyped#36436

But was able to work around it with any. I am going to now squash my commits, this PR is finally good to go

@naseemkullah naseemkullah force-pushed the tracer-start-active-span branch 2 times, most recently from a0e97b5 to 2faa083 Compare May 15, 2021 02:04
@naseemkullah naseemkullah requested a review from dyladan May 15, 2021 02:05
@dyladan
feat: add tracer.startActiveSpan()
81fd6a9 
Add a helper method that starts a new and sets it in the currently
active (or otherwise specified) context and calls a given function
passing it the created span as first argument.

This method is not strictly part of the spec, but the spec does
allow (encourage?) it to be added for convenience:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#span-creation

For best UX, this method has overloads such that it can take in name,fn
or name,opts,fn or name,opts,ctx,fn as args.

These overloads caused some issues with sinon.stub in the proxy-tracer
test but was able to work around it by use of "any". A link to the gh
issue was added as a comment in said test.

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

Co-authored-by: Gerhard Stöbich <deb2001-github@yahoo.de>

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
@naseemkullah naseemkullah force-pushed the tracer-start-active-span branch from 2faa083 to 81fd6a9 Compare May 15, 2021 02:07
@dyladan dyladan added the enhancement New feature or request label May 17, 2021
@dyladan dyladan merged commit 28fabc4 into open-telemetry:main May 17, 2021
@naseemkullah naseemkullah deleted the tracer-start-active-span branch May 17, 2021 17:43
@naseemkullah naseemkullah mentioned this pull request May 17, 2021
Closed
@naseemkullah
Copy link
Member Author

Thanks for the help everyone!

This was referenced May 17, 2021
Closed
Merged
@snyk-bot snyk-bot mentioned this pull request Feb 22, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Flarna Flarna Flarna approved these changes

@MSNev MSNev MSNev approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@dyladan dyladan dyladan approved these changes

@obecny obecny obecny approved these changes

@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

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.
6 participants
@naseemkullah @vmarchaud @Flarna @dyladan @obecny @MSNev