-
Notifications
You must be signed in to change notification settings - Fork 513
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
fix: net instrumentation - change span kind of connect to internal #394
Conversation
@@ -92,7 +92,7 @@ export class NetInstrumentation extends InstrumentationBase<Net> { | |||
/* It might still be useful to pick up errors due to invalid connect arguments. */ | |||
private _startGenericSpan(socket: Socket) { | |||
const span = this.tracer.startSpan('connect', { | |||
kind: SpanKind.CLIENT, | |||
kind: SpanKind.INTERNAL, |
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.
SpanKind.INTERNAL
is the default so you could omit the second argument.
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.
Thanks, good point 🙂 Fixed it
Codecov Report
@@ Coverage Diff @@
## main #394 +/- ##
=======================================
Coverage 94.43% 94.43%
=======================================
Files 11 11
Lines 431 431
Branches 48 48
=======================================
Hits 407 407
Misses 24 24 |
Which problem is this PR solving?
From the spec:
CLIENT
Indicates that the span describes a synchronous request to some remote service. This span is the parent of a remote SERVER span and waits for its response.However there is no corresponding server span for
connect
. Thus I think the least spec violating span kind forconnect
should beINTERNAL
.Short description of the changes
Change span kind of
connect
fromCLIENT
toINTERNAL
.