Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: enhanced typescript support for currentSpan().span #1370

Merged
merged 1 commit into from
Oct 9, 2024
Merged

fix: enhanced typescript support for currentSpan().span #1370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/collector/src/types/shared.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { CollectorConfig } from './collector';
import { GenericLogger, InstanaBaseSpan } from '@instana/core/src/core';

export interface Init {
currentSpan(): InstanaBaseSpan;
currentSpan(): { span: InstanaBaseSpan };
Copy link

@legehwahn legehwahn Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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() {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍

Copy link

@legehwahn legehwahn Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

isTracing(): boolean;
isConnected(): boolean;
setLogger(logger: GenericLogger): void;
Expand All @@ -15,13 +15,13 @@ export interface Init {

export type InitFunction = {
(config?: CollectorConfig): Init;
sdk: any;
currentSpan(): { span: InstanaBaseSpan };
kirrg001 marked this conversation as resolved.
Show resolved Hide resolved
isTracing(): boolean;
isConnected(): boolean;
setLogger(logger: GenericLogger): void;
core: any;
currentSpan: any;
isTracing: any;
isConnected: any;
setLogger: any;
sharedMetrics: any;
experimental: any;
opentracing: any;
sdk: any;
};
2 changes: 1 addition & 1 deletion packages/collector/test/tracing/misc/typescript/ts_cjs/src/app_1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ app.get('/', async (req, res) => {
app.get('/request', async (req, res) => {
const currentSpan = instana.currentSpan();

if (!currentSpan) {
if (!currentSpan || !currentSpan.span || !currentSpan.span.t) {
throw new Error('No current span available.');
}

Expand Down
2 changes: 1 addition & 1 deletion packages/collector/test/tracing/misc/typescript/ts_cjs/src/app_2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ app.get('/', async (req, res) => {
app.get('/request', async (req, res) => {
const currentSpan = instana.currentSpan();

if (!currentSpan) {
if (!currentSpan || !currentSpan.span || !currentSpan.span.t) {
throw new Error('No current span available.');
}

Expand Down
2 changes: 1 addition & 1 deletion packages/collector/test/tracing/misc/typescript/ts_esm/src/app_1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ app.get('/', async (req, res) => {
app.get('/request', async (req, res) => {
const currentSpan = instana.currentSpan();

if (!currentSpan) {
if (!currentSpan || !currentSpan.span || !currentSpan.span.t) {
throw new Error('No current span available.');
}

Expand Down
2 changes: 1 addition & 1 deletion packages/collector/test/tracing/misc/typescript/ts_esm/src/app_2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ app.get('/', async (req, res) => {
app.get('/request', async (req, res) => {
const currentSpan = instana.default.currentSpan();

if (!currentSpan) {
if (!currentSpan || !currentSpan.span || !currentSpan.span.t) {
throw new Error('No current span available.');
}

Expand Down
4 changes: 2 additions & 2 deletions packages/collector/test/tracing/misc/typescript/ts_esm/ts_esm_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const loaderPath = isLatestEsmSupportedVersion(process.versions.node)
mochaSuiteFn('Typescript TS->ESM', function () {
this.timeout(config.getTestTimeout() * 5);

mochaSuiteFn('[CASE 1]', () => {
describe('[CASE 1]', () => {
globalAgent.setUpCleanUpHooks();
const agentControls = globalAgent.instance;

Expand Down Expand Up @@ -68,7 +68,7 @@ mochaSuiteFn('Typescript TS->ESM', function () {
});
});

mochaSuiteFn('[CASE 2]', () => {
describe('[CASE 2]', () => {
globalAgent.setUpCleanUpHooks();
const agentControls = globalAgent.instance;

Expand Down