feat: Add getActiveSpan to trace API

[cartermp]

Based on discussion in #162 I'm opening this up. Both approached discussed in the issue are present -- it'd be great to get to consensus on if we want to do this, and if so, which approach to take.

My preference would be to have the additional getActiveSpan function, but it'd be great to hear what others think.

N.B. the implementations here may be suboptimal; this is mostly just to aid discussion

[codecov]

Codecov Report

Merging #163 (311ab09) into main (bafcf0d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
+ Coverage   95.20%   95.23%   +0.02%     
==========================================
  Files          45       45              
  Lines         646      650       +4     
  Branches      102      102              
==========================================
+ Hits          615      619       +4     
  Misses         31       31              

Impacted Files	Coverage Δ
src/api/trace.ts	100.00% <100.00%> (ø)
src/trace/context-utils.ts	94.11% <100.00%> (+1.26%)	⬆️

---

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 bafcf0d...311ab09. Read the comment docs.

[dyladan]

Great timing I was just looking at your issue this morning. I think a convenience API for users is a great idea and since it can be implemented entirely in the API by calling other APIs it shouldn't even need any SDK changes. I don't think we have a need for both of these new APIs just to avoid confusing our users, but either of them likely provides enough value to justify.

Personally I would probably rather do getCurrentSpan because it does not require changing any existing API which users are already familiar with. Some users may expect getSpan to return undefined if passed an undefined context.

[cartermp]

Agreed, I wouldn't want both either. Shoulda clarified in the PR text that I only want one as well. I also prefer getCurrentSpan, and anecdotally that is also what I've seen people ask for.

[dyladan]

Moving my comment to the issue to keep discussion together.

[dyladan]

You can go ahead and finish this up for review. I'll bring it up in the SIG meeting wednesday

[dyladan]

Please add tests if you can. something like this should work

const span = new NonRecordingSpan();
const ctx = setSpan(Context.ROOT_CONTEXT, span);
context.setGlobalContextManager({ active: () => ctx } as any as ContextManager);

const active = trace.getCurrentSpan();
assert.equals(active, span);

[cartermp]

@dyladan will add a test as well. That's proving a bit more difficult (no mocha experience, weird type errors when importing certain things) but I'll give it a shot. If for some reason I can't figure it out soon I'll let you know.

[cartermp]

Looks like this is the source of 4 new lint warnings and a new circular dependency, hence the linter failing:

I think you may be able to import { context } from '..'; and use that here instead of ContextAPI.getInstance() which will save some small number of bytes when minified.

Moving back to ContextAPI.getInstance() resolves this. Any preference on how to proceed?

Otherwise I think this is ready for review + tested.

[dyladan]

Moving back to ContextAPI.getInstance() resolves this

Circular dependency is not acceptable so if that's the fix then we'll do it

[blumamir]

This is still being discussed in the relevant issue #163 , but if we choose to go with getCurrentSpan option - current implementation LGTM

[legendecas]

Just a side note: it will be great if you can prefix the title of the PR with the type of the changes, like "feat:" for this one, as documented at https://github.com/open-telemetry/opentelemetry-js/blob/main/CONTRIBUTING.md#conventional-commit.

[dyladan]

@cartermp if you can rename the method we can get this merged

[cartermp]

Roger roger

[legendecas]

Should this be a semver-minor or a semver-patch? IIUC, semver-minor makes it incompatible with SDKs not upgraded to the latest version of @opentelemetry/api.

[cartermp]

I guess according to semver this should be a minor update:

MINOR version when you add functionality in a backwards compatible manner

https://semver.org/

But since semver is just a lie that we all agree to tell one another, if there's precedent for adding backwards-compatible features as a patch, then that's not a bad option.

[dyladan]

I guess according to semver this should be a minor update:

MINOR version when you add functionality in a backwards compatible manner

semver.org

Technically yes. It would be a bummer for it to be incompatible because of version and not because of any actual incompatibility though.

semver is just a lie that we all agree to tell one another

😂

We don't actually have any precedent to add features in patch releases but that doesn't mean I'm completely against it. I think the real problem here is we're trying to use one version number to represent the versions of 2 different things. One is the user-facing API and the other is the interface between the API and SDK. This change is minor for the first thing, but actually no change for the second thing. We might be better off not using the public version to track that compatiblity (might be too late to change though).

[cartermp]

Since I'm not a maintainer, take my opinion with a grain of salt, but after sleeping on it I think a semver-minor change is the most appropriate, even though the api and SDK have this special relationship. I think it's a lot more likely that a developer will expect to get new functionality like this in a minor version bump than a patch, especially since it's the official guidance for package authorship that NPM gives too. And even though I'd love for people on older SDKs to get this functionality, they are far outnumbered by the people who will use otel-js for the first time over the next few years (and they will likely update over time too!).