Consider API for explicitly threading span/trace context

[nathanleclaire]

Seems like a lot of people, myself included, have run into issues where the magic context propagation machinery doesn't play nice with async/await. In some cases it can be fixed but in others the limitations of what their code is structured like don't allow them to pull startTrace into a proper parent level scope outside of async.

To fix these instances, it might be nice to have an extension API like startSpanContext, finishSpanContext, withSpanContext etc. that lets users do it manually.

WDYT @toshok

[toshok]

only node native async/await. babel translated versions work.

I'm not sure those methods will be sufficient (or even completely desirable). I'm assuming these methods all take a context parameter that's returned from beeline.getTraceContext().

take for instance:

  // assume we have a trace here
  let user = await fetchUserFromDB(userName);

  let notifications = await fetch(`${notificationApiServer}/user/${user.id}`);

  // custom method call to respond here somehow (render a template/react component, send json, etc.)
  customCode();

if I wanted to make sure that I got my customCode call instrumented, I'd either pass a context in there, or call withSpanContext, or something, e.g.:

  let context = beeline.getTraceContext();

  // assume we have a trace here
  let user = await fetchUserFromDB(userName);

  let notifications = await fetch(`${notificationApiServer}/user/${user.id}`);

  // custom method call to respond here somehow (render a template/react component, send json, etc.)
  beeline.withSpanContext(context, ..., span => {
    customCode();
  });

but the problem is, I still don't have spans from fetch. That might be an instrumented npm package, so it doesn't have any facility for passing context. What I really don't want to see is decorating the entire continuation with a span:

  let context = beeline.getTraceContext();

  // assume we have a trace here
  let user = await fetchUserFromDB(userName);
  beeline.withSpanContext(context, span => {
    let notifications = await fetch(`${notificationApiServer}/user/${user.id}`);

    // custom method call to respond here somehow (render a template/react component, send json, etc.)
    customCode();
  });

[polotek]

Does this issue have a failing test case?

[toshok]

Not currently in the repo. https://github.com/honeycombio/beeline-nodejs/pull/87 has a test that failed for early 10.x versions (it was reverted), but apparently that node bug was fixed in 10.4.0 (see https://github.com/nodejs/node/issues/20274)

[JAForbes]

Would it be suitable to add the trace context to the express request object, but retaining the existing "magic" instrumentation.

That way, simply adding a bit of context in a scenario where the auto instrumentation loses the active trace can be mitigated:

db.begin( async sql => {
  const response = await sql`select x,y,z from table where ...`
  req.beelineTrace.addContext({ ...response })
})

It seems to me, if we have access to the req we should have access to the trace, as they are different representations of the same abstraction.

Maybe to stay idiomatic and avoid null checks against req - we could pass req in as a context object to most beeline methods To avoid coupling with express specifically, the API just requires an object with honeycomb-trace-id on it.

So the following would all work:

// auto detect trace/span (status quo)
beeline.addContext({ new: 'context' })

// specify trace/span with an optional "context object"
const contextObject = { 'honeycomb-trace-id': 1 }
beeline.addContext({ new: 'context' }, contextObject)

// as above, but the "context object" is just `req` because the honeycomb middleware attached the correct trace id
beeline.addContext({ new: 'context' }, req)

Perhaps the context object could be obtained easily too at times when the active trace is not ambiguous. E.g.

const contextObject = beeline.getActiveContext()

I think the auto instrumentation is great for most cases. I'm mainly hitting issues with database transactions with nested async callbacks. It seems logical to me, if I have a reference to the express request, I should also be able to explicitly reference the trace that represents that request.

---

Update: It seems this is already doable with the documented API on unmarshalling traces, though that is not what it was originally intended for.

[vreynolds]

Hello,

We will be closing this issue as it is a low priority for us. It is unlikely that we'll ever get to it, and so we'd like to set expectations accordingly.

As we enter 2022 Q1, we are trimming our OSS backlog. This is so that we can focus better on areas that are more aligned with the OpenTelemetry-focused direction of telemetry ingest for Honeycomb.

If this issue is important to you, please feel free to ping here and we can discuss/re-open.