New issue

Jump to bottom

feat(SugaredTracer): add draft of sugaredTracer #3317

Merged

legendecas merged 8 commits into open-telemetry:main from secustor:implement_sugared_tracer

Jan 16, 2024

+440 −5

Member

secustor commented Oct 10, 2022 •

edited

Loading

Which problem is this PR solving?

Early draft of a SugaredTracer, intended as basis for discussions and implementation ideas.

cc @cartermp
cd @legendecas

Supersedes open-telemetry/opentelemetry-js-api#173
Signed-off-by: secustor sebastian@poxhofer.at

Short description of the changes

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

codecov bot commented Oct 10, 2022 •

edited

Loading

Codecov Report

Merging #3317 (f9ddd18) into main (6898a34) will increase coverage by 1.80%.
The diff coverage is 87.75%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3317      +/-   ##
==========================================
+ Coverage   90.39%   92.19%   +1.80%     
==========================================
  Files         114      336     +222     
  Lines        2363     9512    +7149     
  Branches      527     2016    +1489     
==========================================
+ Hits         2136     8770    +6634     
- Misses        227      742     +515

Files	Coverage Δ
api/src/experimental/index.ts	`100.00% <100.00%> (ø)`
api/src/experimental/trace/SugaredTracer.ts	`87.23% <87.23%> (ø)`

... and 221 files with indirect coverage changes

Member

dyladan commented Oct 10, 2022

Thanks for moving this over

Contributor

cartermp commented Oct 20, 2022

Setting aside the name for a moment -- do we feel that this accomplishes the goal of making it easy to create auto-closing spans as an end-user? If so, then I'd love to try and drive this forward.

Member Author

secustor commented Oct 20, 2022

I have brought this up during the SIG meeting and there hasn't been a negative reaction so far.

Member

legendecas commented Oct 21, 2022

A discussion requesting features that can be solved by this PR: #2477.

secustor marked this pull request as ready for review

November 2, 2022 20:54

secustor requested a review from a team

November 2, 2022 20:54

legendecas reviewed

View reviewed changes

api/src/trace/SugaredTracer.ts Outdated Show resolved Hide resolved

legendecas reviewed

View reviewed changes

api/src/trace/SugaredTracer.ts Outdated Show resolved Hide resolved

legendecas reviewed

View reviewed changes

api/src/trace/SugaredTracer.ts Outdated Show resolved Hide resolved

dyladan reviewed

View reviewed changes

Member

dyladan left a comment

Other than the function name this generally looks good to me

api/src/trace/SugaredTracer.ts Outdated

+               * return a new SugaredTracer created from the supplied one
+               * @param tracer
+               */
+              export function wrapTracer(tracer: Tracer): SugaredTracer {

Member

dyladan Oct 13, 2022

Why this function instead of a user just newing up their own sugared tracer?

Member Author

secustor Nov 21, 2022

My thought has been that we can inject easier logic without touching the constructor itself, but I have no strong feelings regarding this.

api/src/trace/SugaredTracer.ts Outdated Show resolved Hide resolved

api/src/trace/SugaredTracer.ts Outdated

+               * @param arg2
+               * @param arg3
+               */
+              function massageParams<F extends (span: Span) => ReturnType<F>>(arg: SugaredSpanOptions | F,

Member

dyladan Nov 8, 2022

I can't think of a good name for this but the existing name doesn't describe at all what it does. Maybe something like getVariadicParams?

Member Author

secustor Nov 21, 2022

IMO the get suggests that we are retrieving the params somewhere from. What do you think about processParams or normalizeParams?

Contributor

cartermp commented Nov 20, 2022

Assuming names are agreed upon and look good, what all else would be needed to get this accepted and merged?

legendecas reviewed

View reviewed changes

api/src/trace/SugaredTracer.ts Outdated Show resolved Hide resolved

legendecas reviewed

View reviewed changes

api/src/trace/SugaredTracer.ts Outdated Show resolved Hide resolved

legendecas reviewed

View reviewed changes

api/src/trace/SugaredOptions.ts Outdated Show resolved Hide resolved

legendecas reviewed

View reviewed changes

Member

legendecas left a comment

Generally LGTM % nits.

api/CHANGELOG.md Outdated Show resolved Hide resolved

legendecas reviewed

View reviewed changes

api/src/trace/SugaredTracer.ts Outdated Show resolved Hide resolved

legendecas approved these changes

View reviewed changes

Member

dyladan commented Nov 29, 2022

I'm sorry but this has to be held until the conversation in open-telemetry/opentelemetry-specification#2968 is resolved

github-actions bot commented Jan 30, 2023

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions bot added the stale label

Member Author

secustor commented Jan 30, 2023

Still waiting for the outcome...

Contributor

cartermp commented Jan 30, 2023

FWIW I believe that should that spec PR get merged, this is still considered valid as per spec:

Language SIGs MAY provide methods other than End in the API that also end the span to support language-specific features like with statements in Python. However, all API implementations of such methods MUST internally call the End method and be documented to do so.

github-actions bot removed the stale label

Contributor

weyert commented Feb 15, 2023

I have tried it out but I have occasions were the arguments is [fn, undefined, undefined] and then the arguments.length fails. I think we might want to do something like Array.from(arguments).filter(Boolean) to get the correct count?

Member Author

secustor commented Feb 16, 2023

Can you elaborate what function you are referring too? There is no function which has fn as parameter.

Contributor

weyert commented Mar 7, 2023

@secustor Sorry for being unclear I was referring to the massageParams function.

Also any particular reason why it's not part of the class itself?

Member Author

secustor commented Mar 26, 2023

Also any particular reason why it's not part of the class itself?

Idea is to allow usage for potential different classes.

github-actions bot commented Jun 19, 2023

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions bot added the stale label

Member Author

secustor commented Jun 19, 2023

@dyladan @legendecas WDYT about merging this as the specification change hasn't been merged?

If you would, I will fix up this PR.

github-actions bot removed the stale label

Member

legendecas commented Jul 4, 2023 •

edited

Loading

Sorry for the late response.

Regarding the spec changes has been stalled, I'd suggest publishing the sugar tracer in a separate package (e.g. @opentelemetry/api-extension, or @opentelemetry/api-incubator). This is how the Java implementation is doing: https://github.com/open-telemetry/opentelemetry-java/blob/main/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedTracer.java.

In this way, I believe we don’t have to wait for the specification changes and avoid conflicts with future spec APIs.

Member

pichlermarc commented Jul 5, 2023

Sorry for the late response.

Regarding the spec changes has been stalled, I'd suggest publishing the sugar tracer in a separate package (e.g. @opentelemetry/api-extension, or @opentelemetry/api-incubator). This is how the Java implementation is doing: open-telemetry/opentelemetry-java@main/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedTracer.java.

In this way, I believe we don’t have to wait for the specification changes and avoid conflicts with future spec APIs.

We have also discussed another approach recently in the SIG meeting: using different entry points in the API package for experimental features, see #3827 (comment)

Member Author

secustor commented Dec 11, 2023

Can someone assist me with this typedoc lint error?
Not sure how to fix this efficiently.

legendecas reviewed

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

api/package.json Outdated Show resolved Hide resolved

api/package.json Outdated Show resolved Hide resolved

api/src/index.ts Outdated Show resolved Hide resolved

secustor added 4 commits

December 16, 2023 09:32


          feat(api): add experimental package and SugaredTracer

c9d9bc3


          tests(api/experimental): add tests

5b43369


          fix: packages.json exports

4f21c7b


          Merge branch 'main' into implement_sugared_tracer

98d3036

secustor force-pushed the implement_sugared_tracer branch from 93de38e to 98d3036 Compare

December 20, 2023 20:17

secustor added 2 commits

December 20, 2023 21:27


          move export into experimental package

b728872


          add additional tests

b4abbf8

legendecas reviewed

View reviewed changes

Member

legendecas left a comment

Thank you for working on this!

I believe we can still mitigate the api-eol failure as suggested as we'd like to keep runtime support in the API package.

api/src/experimental/trace/SugaredTracer.ts Outdated Show resolved Hide resolved


          fix: do not use catch and finally for compatibility with Node 8

dae0f4d

legendecas approved these changes

View reviewed changes

secustor requested review from dyladan and cartermp

December 22, 2023 20:48

makeavish commented Jan 5, 2024

Since it is approved, can we merge?


          Merge branch 'main' into implement_sugared_tracer

f9ddd18

legendecas merged commit 71ef1b1 into open-telemetry:main

secustor deleted the implement_sugared_tracer branch

January 16, 2024 09:55

pksunkara mentioned this pull request

Add a withSpan (or similar) convenience API #3250

Closed

pichlermarc mentioned this pull request

chore: prepare release API 1.8.0/Core 1.21.0/Experimental 0.49.0 #4504

Merged

This was referenced May 22, 2024

[Snyk] Upgrade @opentelemetry/api from 1.6.0 to 1.8.0 Hawthorne001/edge-logger#2

Open

[Snyk] Upgrade @opentelemetry/api from 1.6.0 to 1.8.0 Hawthorne001/edge-logger#3

Open

integraci-co804 mentioned this pull request

[Snyk] Upgrade @opentelemetry/api from 1.6.0 to 1.8.0 integraci-co804/nodejs-app#5

Open

Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request


          feat(SugaredTracer): add draft of sugaredTracer (open-telemetry#3317)

61361d7

* feat(api): add experimental package and SugaredTracer

* tests(api/experimental): add tests

* fix: packages.json exports

* move export into experimental package

* add additional tests

* fix: do not use catch and finally for compatibility with Node 8

---------

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet