Add a withSpan (or similar) convenience API

[dyladan]

We can close this issue since the question is addressed and we can create new issues for proposed convenience APIs

Originally posted by @dyladan in open-telemetry/opentelemetry-js-api#164 (comment)

I propose we add an API which takes a callback, automatically creating and ending a span. This has been proposed in the past and blocked for administrative reasons explained in the previously linked ticket, but I think now is the time to address it.

[vmarchaud]

For reference, we made those utils at my company:

import * as otelAPI from '@opentelemetry/api'
import * as otelCore from '@opentelemetry/core'

export function withSpan <T extends () => ReturnType<T>> (span: otelAPI.Span, fn: T): ReturnType<T> {
  return otelAPI.context.with(
    otelAPI.trace.setSpan(otelAPI.context.active(), span),
    fn
  )
}

export function withoutSpan <T extends () => ReturnType<T>> (fn: T): ReturnType<T> {
  return otelAPI.context.with(
    otelCore.suppressTracing(otelAPI.context.active()),
    fn
  )
}

export function newSpan (spanName: string, attributes: otelAPI.SpanAttributes) {
  return otelAPI.trace.getTracer('default').startSpan(spanName, {
    attributes
  })
}

type Unpromisify<T> = T extends Promise<infer U> ? U : T
export async function withNewSpan <T extends () => ReturnType<T>> (
  spanName: string,
  attributes: otelAPI.SpanAttributes,
  fn: T
): Promise<Unpromisify<ReturnType<T>>> {
  const span = newSpan(spanName, attributes)
  // note: we await even if not needed to ensure we end() the span when the function
  // as been fully executed
  // tslint:disable-next-line: await-promise
  const result = await withSpan(span, fn)
  span.end()
  return result as any
}

export function withNewSpanSync <T extends () => ReturnType<T>> (
  spanName: string,
  attributes: otelAPI.SpanAttributes,
  fn: T
): ReturnType<T> {
  const span = newSpan(spanName, attributes)
  const result = withSpan(span, fn)
  span.end()
  return result as any
}

[dyladan]

I wonder if getTracer should return a "sugared" tracer which implements convenience APIs so that we don't have to implement in SDK (and bump required version)?

export class SugaredTracer implements Tracer {
  constructor(private _tracer: Tracer) {
    this.startSpan = _tracer.startSpan.bind(_tracer);
    this.startActiveSpan = _tracer.startActiveSpan.bind(_tracer);
  }

  startActiveSpan: Tracer['startActiveSpan'];
  startSpan: Tracer['startSpan'];

  withSpan <F extends (span: Span) => unknown>(name: string, options: SpanOptions, fn: F): ReturnType<F> {
    const span = this._tracer.startSpan(name, options);
    const ret = fn(span);
    if (ret instanceof Promise) {
      return ret.finally(() => span.end()) as ReturnType<F>;
    }
    span.end();
    return ret as ReturnType<F>;
  }
}

[Flarna]

Above helpers require also @opentelemetry/core which would be problematic for API package.

[cartermp]

Another decision point would be if such a helper made the span the active span or not (and/or if there should be an equivalent withActiveSpan if not).

Might be helpful to look at other languages that do this too:

• Ruby uses a @tracer.in_span where you get a lambda parameter and it's auto-closed for you. It's explicitly called out in the API docs as the default you should use as an end-user.
• Rust does the same as Ruby, and although it's not called out as the default the same way as Ruby, it's so much better than the alternative (you get to manage move semantics, whee!) that it's likely a de facto default. When you use in_span, the span in the lambda is the currently active span.
• C++ introduces a scope-based lifetime when you mark a span as active, avoiding the need to close the span unless you must close it prior to going out of scope
• Python extends the resource management metaphor of with to spans and has syntactical decorators to accomplish this, which are both not applicable for JS
• .NET has resource management like Python (using/use) and similarly extends the metaphor to spans

[dyladan]

Above helpers require also @opentelemetry/core which would be problematic for API package.

only withoutSpan

[dyladan]

Another decision point would be if such a helper made the span the active span or not (and/or if there should be an equivalent withActiveSpan if not).

Might be helpful to look at other languages that do this too:

• Ruby uses a @tracer.in_span where you get a lambda parameter and it's auto-closed for you. It's explicitly called out in the API docs as the default you should use as an end-user.
• Rust does the same as Ruby, and although it's not called out as the default the same way as Ruby, it's so much better than the alternative (you get to manage move semantics, whee!) that it's likely a de facto default. When you use in_span, the span in the lambda is the currently active span.
• C++ introduces a scope-based lifetime when you mark a span as active, avoiding the need to close the span unless you must close it prior to going out of scope
• Python extends the resource management metaphor of with to spans and has syntactical decorators to accomplish this, which are both not applicable for JS
• .NET has resource management like Python (using/use) and similarly extends the metaphor to spans

I think most of the time you're trying to wrap a callback in a span you would want it to be active in the callback. Almost by definition everything that happens within the callback would be a child of the callback operation and should be reflected that way in tracing data.

[cartermp]

Any concerns with implementing this? I'd be happy to contribute the sugared tracer design if that's preferred (it sounds like that would go in this repo and not the SDK repo?): #3250

[secustor]

@cartermp Are you working on this?

I would really like to see a SugaredTracer with some additional convenience functions.

If not, I would start to look into it myself.

[cartermp]

@secustor I haven't started it no - was looking to see if anyone felt that was the right direction to go. But if you're keenly interested as well, I'd be more than happy to help review and discuss its usage. I too would love to see this added.

[dyladan]

Moving this to the main JS repository since we have decided to merge the repositories together.

[cartermp]

Thinking about if we could just make startActiveSpan call end(). I guess that's a breaking change though:

let sp = tracer.startActiveSpan("ugh", span => {
  // something something something
  return span
});

// This would now be a no-op if it were ended after the call to `startActiveSpan`
sp.addEvent("yippee");

sp.end();

Would that mean that any changes to startActiveSpan to call end() internally are now a no-go?

[dyladan]

It depends on your definition of "break" since the API itself isn't really changing, but assuming we agree that would be a breaking change then that's the way I understand it yes. But the name startActiveSpan to me doesn't imply end will be called anyway. For the record I think I would consider this a breaking change.

I think your code sample has the comment on the wrong line :)

[brett-vendia]

Here's our wrapper in case it's useful to others (would appreciate feedback). It has automatic span ending and error logging.

export function spanify(
  fn: (...params: any) => any,
  spanOptions: SpanOptions = {},
  spanName: string = fn.name.replace(/^_/, '')
) {
  return (...functionArgs) => {
    return tracer.startActiveSpan(spanName, spanOptions, async (span) => {
      try {
        const result = await fn(...functionArgs, span)
        span.setStatus({
          code: SpanStatusCode.OK,
        })
        return result
      } catch (error: any) {
        const errorMessage = String(error)
        diag.error(`${spanName}:ERROR`, {
          errorMessage,
        })
        span.setStatus({ code: SpanStatusCode.ERROR, message: errorMessage })
        span.recordException(error)
        throw error
      } finally {
        span.end()
      }
    })
  }
}

function _foo(a, b, c, span) {
  span.setAttribute('foo', 'bar')
}

export const foo = spanify(_foo)

[Compulsed]

We have tried to make an improvement on the above ^

function spanify<F extends (args: Parameters<F>[0]) => ReturnType<F>>(
    fn: F,
    spanOptions: SpanOptions = {},
    spanName: string = fn.name.replace(/^_/, '')
  ): (args: Omit<Parameters<F>[0], 'span'>) => Promise<ReturnType<F>> {
    return (functionArgs) => {
      return tracer.startActiveSpan(spanName, spanOptions, async (span: Span): Promise<ReturnType<F>> => {
        try {
          const result = await fn({ ...functionArgs, span })
          
          span.setStatus({
            code: SpanStatusCode.OK,
          })
          
          return result
        } catch (error: any) {
          const errorMessage = String(error)
  
          if (!error.spanExceptionRecorded) {
            span.recordException(error)
            span.setStatus({ code: SpanStatusCode.ERROR, message: errorMessage })
            error.spanExceptionRecorded = true
          }
  
          throw error
        } finally {
          span.end()
        }
      })
    }
  }
  
  const getCount = spanify(async function getCounter({ span }: { span: Span }): Promise<number> {
    const data = await ddbDocClient.send(new GetCommand({
        TableName: process.env.DDB_TABLE_NAME,
        Key: {
            id: COUNTER_KEY
        }
    }));

    const currentCount = data?.Item?.count || 0

    span.setAttributes({ currentCount })

    return currentCount
})

[github-actions]

This issue 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.

[tuchandra]

👋🏽 I still consider this to be a need that's unmet by the JS tooling; the ability to use @tracer.start_as_active_span as a decorator in Python makes it much easier for folks to instrument their code.

[secustor]

👋🏽 I still consider this to be a need that's unmet by the JS tooling; the ability to use @tracer.start_as_active_span as a decorator in Python makes it much easier for folks to instrument their code.

This is currently blocked by specification discussions. See the linked PR.

[pksunkara]

Would #3827 solve this issue?

[tuchandra]

Would #3827 solve this issue?

I think so. I adapted the code proposed in the comments for my own company's instrumentation helpers.

[cartermp]

It kind of does. It doesn't solve the issue of wanting an auto-closing span within a function:

function doot() {
  withSpan((span) => {
    // track an operation
  });
  // do something else
  withSpan((span) => {
    // track a different operation
  });
}

But it does likely eliminate a lot of the need for such a convenience API.

[pksunkara]

I think this can be closed since #3317 has been merged.

[github-actions]

This issue 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.

[github-actions]

This issue was closed because it has been stale for 14 days with no activity.