feat(SugaredTracer): add draft of sugaredTracer

[secustor]

trafficstars

Which problem is this PR solving?

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

Ref https://github.com/open-telemetry/opentelemetry-js/issues/3250

cc @cartermp cd @legendecas

Supersedes https://github.com/open-telemetry/opentelemetry-js-api/pull/173 Signed-off-by: secustor [email protected]

Short description of the changes

Type of change

Please delete options that are not relevant.

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] 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:

• [x] Followed the style guidelines of this project
• [ ] Unit tests have been added
• [ ] Documentation has been updated

[codecov[bot]]

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

[dyladan]

Thanks for moving this over

[cartermp]

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.

[secustor]

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

[legendecas]

A discussion requesting features that can be solved by this PR: https://github.com/open-telemetry/opentelemetry-js/discussions/2477.

[cartermp]

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

[dyladan]

I'm sorry but this has to be held until the conversation in https://github.com/open-telemetry/opentelemetry-specification/pull/2968 is resolved

[github-actions[bot]]

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.

[secustor]

Still waiting for the outcome...

[cartermp]

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.

[weyert]

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?

[secustor]

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

[weyert]

@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?

[secustor]

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

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.

[secustor]

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

If you would, I will fix up this PR.

[legendecas]

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.

[pichlermarc]

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 https://github.com/open-telemetry/opentelemetry-js/issues/3827#issuecomment-1612525131

[secustor]

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

[makeavish]

Since it is approved, can we merge?