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.

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

Span parent ids not being allocated correctly #32

Closed
peternewnham opened this issue Apr 1, 2022 · 2 comments
Closed

Span parent ids not being allocated correctly #32

peternewnham opened this issue Apr 1, 2022 · 2 comments
Assignees
Labels
bug Something isn't working under-investigation

Comments

@peternewnham
Copy link
Contributor

I'm using the module (which is great btw), however I've noticed that parent ids are not being allocated correctly to nested spans.

With a simple app that imports the default OpenTelemetryModule module and has a controller that calls some service methods, the spans that are generated all have a parentId of the root span instead of their actual parents:

HTTP (id: 1, parentId: undefined)
-> Controller (id: 2, parentId: 1)
  -> Service1 (id: 3, parentId: 1)    - parentId should be 2
    -> Service3 (id: 5, parentId: 1)  - parentId should be 3
  -> Service2 (id: 4, parentId: 1)    - parentId should be 2

This means that spans aren't being grouped correctly.

I've applied a patch to my local version which corrects this behaviour and am happy to submit a PR here as well but wanted to get an opinion on it first in case there's something fundamentally wrong with what i'm doing that i'm not aware of 😄

class BaseTraceInjector {
  protected wrap() {
    const method = {
      [prototype.name]: function (...args: any[]) {
        const tracer = trace.getTracer('default');

        // this is the offender - trace.getSpan(context.active()) always returns the top level 'default' span
        //const currentSpan = trace.getSpan(context.active()) ?? tracer.startSpan('default');

        // instead create the new span with the traceName outside of context.with and use that to create the context
        // all nested spans will now be allocated this span as their parent
        const span = tracer.startSpan(traceName);

        return context.with(
          trace.setSpan(context.active(), span),
          () => {
            // no need to create the span inside here now
            //const span = tracer.startSpan(traceName);

           // everything else stays the same
  }
}
@MetinSeylan
Copy link
Owner

Hi, @peternewnham thank you for your kind words. Your point is correct I tested it in my local. Can you open pr?

@MetinSeylan
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working under-investigation
Projects
None yet
Development

No branches or pull requests

2 participants