Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

feat: add tracer.startActiveSpan() helper #14

Closed
wants to merge 1 commit into from
Closed

feat: add tracer.startActiveSpan() helper #14

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
24 changes: 23 additions & 1 deletion src/context/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import { Context } from './types';
import { Baggage, Span, SpanContext } from '../';
import { Baggage, Span, SpanContext, context } from '../';
import { NoopSpan } from '../trace/NoopSpan';

/**
Expand Down Expand Up @@ -55,6 +55,28 @@ export function setSpan(context: Context, span: Span): Context {
return context.setValue(SPAN_KEY, span);
}

/**
* Helper to execute the given function on a new context created from
* active context with given span set.
* Note: The global context manager is used to get active context.
* @param span span to set on context
* @param fn function to execute in new context
* @param thisArg optional receiver to be used for calling fn
* @param args optional arguments forwarded to fn
*/
export function withSpan<
A extends unknown[],
F extends (...args: A) => ReturnType<F>
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if F was also passed the span instance so you could write code like

withSpan(tracer.startSpan('name'), async (span: Span) => {
  // ...
});

and avoid having to assign the span to a variable first like:

let span = tracer.startSpan('foo');
withSpan(span, () => {...});

span = tracer.startSpan('bar');
withSpan(span, () => {...});

Copy link
Member

Choose a reason for hiding this comment

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

The main idea of putting a span into a context and set this context active is to avoid the need to pass spans around manually. Instead you fetch it via getSpan(context.active()).

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, I don't mean so it's available to pass around to other functions. I just mean as a convenience in the "with" scope to avoid boilerplate like getSpan(context.active()).setAttribute('foo', 'bar') everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

The recommended solution for that would be

withSpan(tracer.startSpan('name', { attributes }), async () => {
  // ...
});

Copy link
Member

Choose a reason for hiding this comment

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

Isn't a closure scope exactly providing what you want?

const span = tracer.startSpan('name');
withSpan(span, async () => {
  // ... span is accessible here
});

The current with() proposal allows to use any existing function without modification as you can pass thisArg and arguments as needed. Having a requirement to accept a span as first argument would be quite limiting or requires everyone to implement wrappers/adaptors.

Copy link
Member

@aabmass aabmass Apr 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@dyladan right, but often you want to set attributes and span status, events, etc. conditionally and then always finally span.end().

I'm not claiming it's not possible to do as is, but this PR is intended to be a convenience API so why not make it even more convenient? It also avoids having to name a bunch of variables and have them all in scope when dealing with concurrent operations (which could be pretty error prone):

const span1 = tracer.startSpan('part1');
const span2 = tracer.startSpan('part2');
await Promise.all(
  withSpan(span1, async () => {
    // ...
    span1.end();
  }),
  withSpan(span2, async () => {
    // ...
    span2.end();
  })
);

vs

await Promise.all(
  withSpan(tracer.startSpan('part1'), async (span: Span) => {
    // ...
    span.end();
  }),
  withSpan(tracer.startSpan('part2'), async (span: Span) => {
    // ...
    span.end();
  })
);

>(
span: Span,
fn: F,
thisArg?: ThisParameterType<F>,
...args: A
): ReturnType<F> {
const ctx = setSpan(context.active(), span);
return context.with(ctx, fn, thisArg, ...args);
}

/**
* Wrap span context in a NoopSpan and set as span in a new
* context
Expand Down
55 changes: 55 additions & 0 deletions test/context/context.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,44 @@
import * as assert from 'assert';
import {
createContextKey,
getSpan,
isInstrumentationSuppressed,
ROOT_CONTEXT,
suppressInstrumentation,
unsuppressInstrumentation,
withSpan,
} from '../../src/context/context';
import { NoopSpan } from '../../src/trace/NoopSpan';
import { Context, context, NoopContextManager } from '../../src';

const SUPPRESS_INSTRUMENTATION_KEY = createContextKey(
'OpenTelemetry Context Key SUPPRESS_INSTRUMENTATION'
);

// simple ContextManager supporting sync calls (NoopContextManager.active returns always ROOT_CONTEXT)
class SimpleContextManager extends NoopContextManager {
active(): Context {
return this._activeContext;
}

with<A extends unknown[], F extends (...args: A) => ReturnType<F>>(
context: Context,
fn: F,
thisArg?: ThisParameterType<F>,
...args: A
): ReturnType<F> {
const prevContext = this._activeContext;
try {
this._activeContext = context;
return fn.call(thisArg, ...args);
} finally {
this._activeContext = prevContext;
}
}

private _activeContext = ROOT_CONTEXT;
}

describe('Context Helpers', () => {
describe('suppressInstrumentation', () => {
it('should set suppress to true', () => {
Expand Down Expand Up @@ -78,4 +106,31 @@ describe('Context Helpers', () => {
});
});
});

describe('withSpan', () => {
before(() => {
const mgr = new SimpleContextManager();
context.setGlobalContextManager(mgr);
});

after(() => {
context.disable();
});

it('should run callback with span on context', () => {
const span = new NoopSpan();

function fnWithThis(this: string, a: string, b: number): string {
assert.strictEqual(getSpan(context.active()), span);
assert.strictEqual(this, 'that');
assert.strictEqual(arguments.length, 2);
assert.strictEqual(a, 'one');
assert.strictEqual(b, 2);
return 'done';
}

const res = withSpan(span, fnWithThis, 'that', 'one', 2);
assert.strictEqual(res, 'done');
});
});
});
6 changes: 6 additions & 0 deletions test/internal/global.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ describe('Global Utils', () => {
delete _globalThis[Symbol.for('io.opentelemetry.js.api.0')];
});

afterEach(() => {
api1.context.disable();
api1.propagation.disable();
api1.trace.disable();
});

it('should change the global context manager', () => {
const original = api1.context['_getContextManager']();
const newContextManager = new NoopContextManager();
Expand Down