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

Feature: Implement @jambonz/tracing library #311

Open
wants to merge 79 commits into
base: main
Choose a base branch
from

Conversation

ajukes
Copy link
Contributor

@ajukes ajukes commented Apr 11, 2023

Move tracing to a new library and implement into Feature Server.

  • For inbound calls extract X-Trace-ID | X-Span-ID from Sip Header
  • Add X-Trace-ID | X-Span-ID to outgoing sip headers
  • Simplify logging/debugging as all span creation is within same class
  • This should allow us to have uncoupled servers/services under the same traceId in Jaeger.

Example Trace
image

@davehorton davehorton force-pushed the feature/override-trace-id branch from 9616c55 to 177cd4b Compare April 11, 2023 16:49
@davehorton
Copy link
Contributor

I'm not quite clear on how this works. You've added two new files but no changes to tracer.js and I can't see these files referenced anywhere

@davehorton
Copy link
Contributor

also can you confirm this works with inbound calls as well as REST api triggered outdials?

@ajukes
Copy link
Contributor Author

ajukes commented Apr 11, 2023

SipPropagator works similar to B3Propagator. here

Using the extract method to return a Context which then is used to create the span. I mean I could simply do the following in the call-tracer.js file

const ctx = trace.setSpanContext(context, {
      traceId,
      spanId,
      isRemote: true,
      traceFlags: TraceFlags.SAMPLED
    });

But it seemed cleaner to do the extracting of traceId, spanId in the SipPropagator class. I haven't hooked this into the tracer config but I suppose that could be the next natural step. Then instead of manually instantiating SipPropagator we could just leverage propagation.extract(context.active(), input); seen here

What are your thoughts? Unless I have completely missed the mark here.

I haven't tested REST calls yet, I will update the PR once tested.

@ajukes
Copy link
Contributor Author

ajukes commented Apr 12, 2023

@davehorton Can you confirm that the traces for outbound REST calls were successfully being propagated before this PR? I have a feeling they wasn't as all outbound calls (regardless whether INBOUND or REST initiated the call) have a trace id of "0000000000000000" - I think there is an underlying bug here separate to this PR.

@ajukes
Copy link
Contributor Author

ajukes commented Apr 12, 2023

I can see REST call in jaeger with a traceID representing the callSid so the propagator works as expected. However, what jambonz saves to the DB is "000000000000.."

image

@davehorton
Copy link
Contributor

can you set this PR to draft status for now?

@ajukes ajukes marked this pull request as draft April 13, 2023 08:43
@ajukes
Copy link
Contributor Author

ajukes commented May 9, 2023

@davehorton I just rebased but am unsure if I implemented correctly. let me know if I need to redo.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants