fix: enhanced typescript support for currentSpan().span

[kirrg001]

import instana from '@instana/collector';

....

// type error because it was not defined in shared.d.ts
instana.currentSpan().span

FYI: We are aware that we have more any definitions. We can optimize the TS support later.

[aryamohanan]

LGTM

[legehwahn]

Should named / destructured imports also be supported?
By the looks of it, it is currently possbile via the default export type:

export type InitFunction = {
  (config?: CollectorConfig): Init;
  sdk: any;
  core: any;
  currentSpan: { span: InstanaBaseSpan };
  isTracing: boolean;
  isConnected: boolean;;
  setLogger: (logger: GenericLogger): void;
  sharedMetrics: any;
  experimental: any;
  opentracing: any;
}

e.g.

import { currentSpan } from '@instana/collector';
currentSpan().span;

Can add another esm test if need be.

[legehwahn]

Though the following feedback is not within the scope of the change, I believe it is worth considering.

For users wanting to access the currentSpan, nesting the InstanaBaseSpan under a span property adds no benefit, it adds an additional property getter call at runtime to get to the desired current span object.
Consider removing this level of the response in the actual implementation unless the intent is to extend the currentSpan response type in future to include more than the InstanaBaseSpan type.

[legehwahn]

This is lacking the type for the scenario when the collector / tracer is not initialized / when the call is not being traced.
Suspect it should be:

currentSpan(): { span: InstanaBaseSpan | undefined };

The above is based on the call chain for currentSpan()

@instana/collector/src/index.js

init.currentSpan = function getHandleForCurrentSpan() {
  return instanaNodeJsCore.tracing.getHandleForCurrentSpan();
};

@instana/core/src/tracing/index.js

exports.getHandleForCurrentSpan = function getHandleForCurrentSpan() {
  return spanHandle.getHandleForCurrentSpan(cls);
};

@instana/core/src/tracing/spanHandle.js

/**
 * @param {import ('./cls')} cls
 * @returns {SpanHandle | NoopSpanHandle}
 */
exports.getHandleForCurrentSpan = function getHandleForCurrentSpan(cls) {
  if (cls && cls.isTracing()) {
    return new SpanHandle(cls.getCurrentSpan());
  } else {
    return new NoopSpanHandle();
  }
};

/**
 * TODO: make it as a class
 * Provides noop operation for the SpanHandle API when automatic tracing is not enabled or no span is currently active.
 */
function NoopSpanHandle() {}

[kirrg001]

I understand what you mean in general, but the differentiation if the span is available happens on runtime and should not affect the TS type checking.

If you are running into a TS error, please share here with us 👍

[legehwahn]

Thanks for acknowledging.
I'm in agreement, it does not affect TS type checking - TS would indicate the code should run without any type errors.
This is not necessarily the truth if there is misalignment between the type definition and the actual JS implementation.

Here's an example of a code snippet that would produce a runtime error, without producing a warning / error during tsc build.

The TypeScript compiler has no issue with this code as it does not know the value can actually be undefined at runtime and therefore does not warn the developer during tsc build.

import { type InstanaBaseSpan } from '@instana/core/src/core.js';
import instana from '@instana/collector';

// Re-usable function to get subset of fields from the span to use through-out the application.
function getLogDetailFromSpan(span: InstanaBaseSpan) {
  return span.t; // TypeError: Cannot read properties of undefined (reading 't')
}

// Explicitly specifying the type here, but this is the type which TS would derive anyway.
const currentSpan: { span: InstanaBaseSpan } = instana.currentSpan();
const traceId = getLogDetailFromSpan(currentSpan);

The code produces an error at runtime when the collector is not initialized (which is the case for me during local development) or when the current call isn't traced by the collector.

[kirrg001]

Your code snippet is not right.

instana.currentSpan();

Either returns a SpanHandle or NoopSpanHandle.

NoopSpanHandle {} or SpanHandle { span }

You could protect your code like this:

...

const spanHandle = instana.currentSpan();

// Option 1
if (spanHandle.constructor.name === 'NoopSpanHandle') {}

// Option 2
if (!spanHandle.span) {}

Keep in mind: There should be always an active span in an async context.

app.get('something', (req) => {
   const span = instana.currentSpan();
   // span should exist
})

Either returns a SpanHandle or NoopSpanHandle.

We definitely can improve our SDK. It would be much better if customers could simply do !instana.currentSpan(). A PR is welcome to improve this without breaking the old notation.

[kirrg001]

@legehwahn See also #1374. Another improvement, which makes it easier to access the information of a span.

[kirrg001]

For users wanting to access the currentSpan, nesting the InstanaBaseSpan under a span property adds no benefit, it adds an additional property getter call at runtime to get to the desired current span object.
Consider removing this level of the response in the actual implementation unless the intent is to extend the currentSpan response type in future to include more than the InstanaBaseSpan type.

We are aware of that. This is on our list of breaking changes for a future major release. Outside of this PR scope.

[kirrg001]

Should named / destructured imports also be supported?
By the looks of it, it is currently possbile via the default export type:

export type InitFunction = {
(config?: CollectorConfig): Init;
sdk: any;
core: any;
currentSpan: { span: InstanaBaseSpan };
isTracing: boolean;
isConnected: boolean;;
setLogger: (logger: GenericLogger): void;
sharedMetrics: any;
experimental: any;
opentracing: any;
}
e.g.

import { currentSpan } from '@instana/collector';
currentSpan().span;
Can add another esm test if need be.

In general: yes, we definitely could enhance our TS support in many ways. But again, outside of this PR.

We are happy for any help & contribution pull requests :)