feat: enable root span to contain route #327
Conversation
Codecov Report
@@ Coverage Diff @@
## main #327 +/- ##
=======================================
Coverage 94.34% 94.34%
=======================================
Files 10 10
Lines 407 407
Branches 44 44
=======================================
Hits 384 384
Misses 23 23 |
| @@ -118,7 +121,8 @@ export class KoaInstrumentation extends BasePlugin<typeof koa> { | |||
| middlewareLayer[kLayerPatched] = true; | |||
| this._logger.debug('patching Koa middleware layer'); | |||
| return async (context: KoaContext, next: koa.Next) => { | |||
| if (api.getSpan(api.context.active()) === undefined) { | |||
| const parent = api.getSpan(api.context.active()) as KoaPluginSpan; | |||
There was a problem hiding this comment.
i'm not sure why you are extending the span api, can't you use context() instead ?
There was a problem hiding this comment.
I have to find out somehow who is the root parent span.. I do it by checking that the parentSpanId is undefined..
There was a problem hiding this comment.
I remembered that we do the same in the express plugin so thats fine
There was a problem hiding this comment.
This isn't safe. An instrumentation may be used by a third party SDK which doesn't have the same private properties we do. The safer (but more roundabout) way to check if it is the root would be to check if there is a span in context before the first span is created
|
@vmarchaud Hi ! |
|
@shyimo sorry for the delay, could you rebase the PR ? |
DinaYakovlev
force-pushed
the
koa-enhancment
branch
from
61af54c
to
3b09b3d
Compare
|
@vmarchaud rebased, please have another look :) |
DinaYakovlev
force-pushed
the
koa-enhancment
branch
from
5add4e8
to
a02566c
Compare
|
@DinaYakovlev Thanks for the contribution and sorry for waiting so long |
Which problem is this PR solving?
In addition to #176 PR and to #273 We should able to instrument the root span name. This way it would be clear from the beginning on what request the span is about, and if in the future there would be an option to remove layers (like in the express plugin) the route will be in the root span.
Short description of the changes
using koa plugin before the change:
using koa plugin after the change: