Only set params on Koa router layer #757
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✔️ All good! |
unflxw
force-pushed
the
fix-koa-span-params
branch
2 times, most recently
from
1a8f3f7 to
086d959
Compare
unflxw
changed the title
luismiramirez
approved these changes
Nov 3, 2022
To avoid setting the params in many different spans, potentially interfering with custom instrumentation on those same params, only set the params on the router layer.
Update to the latest version of `@opentelemetry/instrumentation-koa` for the request hook info to contain the layer type.
Not every instrumentation makes the spans that they're working with into active spans for the current context. Additionally, some instrumentations provide hooks where the relevant span is provided as an argument, but is not the active span. To account for these scenarios, the helpers now take an optional span argument as their final argument (except for sendError, which always creates a new span)
When the Koa instrumentation request hook is executed, the current active span is the span from the _previous_ layer. This means that, for the router layer, the active span is the last middleware in the middleware chain. While this is fine functionally, it makes the behaviour of the hook hard to reason about. This change makes it so that the params are set in the span passed to the hook, instead of in the active span. The integration tests are also amended to check for the params in the router child span.
unflxw
force-pushed
the
fix-koa-span-params
branch
from
086d959 to
295858e
Compare
unflxw
added a commit
that referenced
this pull request
Nov 7, 2022
In #757, the signature of the helpers was changed to add an optional span argument. If present, the custom attribute or event set by the helper will be set in the given span, instead of on the current active span. I meant to add tests for it when making the change, but then I forgot. Here are the tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Only set params on router layer
To avoid setting the params in many different spans, potentially
interfering with custom instrumentation on those same params, only
set the params on the router layer.
Bump Koa instrumentation version
Update to the latest version of
@opentelemetry/instrumentation-koafor the request hook info to contain the layer type.
Allow span to be passed to helpers
Not every instrumentation makes the spans that they're working
with into active spans for the current context. Additionally,
some instrumentations provide hooks where the relevant span is
provided as an argument, but is not the active span.
To account for these scenarios, the helpers now take an optional
span argument as their final argument (except for sendError, which
always creates a new span)
Pass span to params helper in Koa hook
When the Koa instrumentation request hook is executed, the current
active span is the span from the previous layer. This means that,
for the router layer, the active span is the last middleware in the
middleware chain.
While this is fine functionally, it makes the behaviour of the hook
hard to reason about. This change makes it so that the params are
set in the span passed to the hook, instead of in the active span.
The integration tests are also amended to check for the params in
the router child span.