-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat(collector): nats 2.x #702
Conversation
f42cee9
to
b6ae7da
Compare
- added trace correlation
testSubscribe.call(this, withError); | ||
}); | ||
// NOTE: We do not track these errors yet, because there is no entry span | ||
describe.skip('connect', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@basti1302 Never noticed that we do not trace connection errors for instrumentations. Is that something we want to trace in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, well, as you wrote: Usually this happens outside of active tracing, in the startup phase of the application, before it even starts to process incoming requests. The general rule "we don't trace an exit if there is no active entry span" still holds. So specifically for NATS I think we are fine without tracing this?
If, on the other hand, we would instrument a driver that typically establishes a connection while processing an incoming request, we would probably want to instrument that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general rule "we don't trace an exit if there is no active entry span" still holds.
Yes. The question is: Is that something we want to trace in the future in general for all instrumentations (independent of the engine)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK there are no plans to change this rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! There is one thing that I think needs to be changed before merging (see comment)
if (cls.skipExitTracing({ isActive })) { | ||
addSuppressionHeaders(originalArgs, isLatest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bug: I think we need to check the result of skipExitTracing whether it has the skipTracingResult.suppressed
flag set. If we skip tracing this call for other reasons, we must not add any headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good point. Although there is no real use case or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case is skipping the tracing because the collector has not yet connected to the agent. The downstream service might also be instrumented and already connected to an agent, in this case the expected behavior is that the downstream service starts a trace with a root span. If we send X-INSTANA-L=0 downstream incorrectly, the downstream tracer would suppress instead of starting a trace.
refs 99434
.connect({callback: ... })