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

feat: use Koa router name as span name if available #976

Merged
merged 4 commits into from
Jun 22, 2022

Conversation

lukiano
Copy link
Contributor

@lukiano lukiano commented Apr 19, 2022

Which problem is this PR solving?

  • This allows to use the Koa router name (if available) as the span name. Usually this name is more meaningful than "method + path". In particular if the route name is the same as the OpenAPI's operationId for that route then it will be easier to associate spans with the OpenAPI spec.

Short description of the changes

  • Koa will store the route name in _matchedRouteName. Note that I'm not using the format router - xxx as I'd rather have the span name clean.

Checklist

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.
    NPM failed to compile on the untouched main branch (some type errors related to React)

@lukiano lukiano requested a review from a team April 19, 2022 05:28
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 19, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: lukiano / name: Luciano Leggieri (97952aa)

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #976 (81ac2c3) into main (a8c5c9f) will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #976      +/-   ##
==========================================
+ Coverage   95.91%   96.20%   +0.28%     
==========================================
  Files          13       18       +5     
  Lines         856     1000     +144     
  Branches      178      203      +25     
==========================================
+ Hits          821      962     +141     
- Misses         35       38       +3     
Impacted Files Coverage Δ
...ode/opentelemetry-instrumentation-koa/src/utils.ts 100.00% <100.00%> (ø)
...lemetry-instrumentation-koa/src/instrumentation.ts 97.18% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 98.00% <0.00%> (ø)
...ode/opentelemetry-instrumentation-koa/src/types.ts 100.00% <0.00%> (ø)
...ry-instrumentation-koa/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)

@rauno56
Copy link
Member

rauno56 commented Apr 26, 2022

@lukiano, please sign the CLA.

@lukiano
Copy link
Contributor Author

lukiano commented Apr 28, 2022

I've signed the CLA

@rauno56
Copy link
Member

rauno56 commented May 2, 2022

A named route refers to a route + a method so there's no way to use a named route with different methods?

  1. The test should not be edited. Add another one for your usecase.
  2. Nit: Your test says "insertOperation" even though it's a get operation for a post(not to mistaken for a POST method).
  3. Current implementation already does gymnastics with naming the span, move your logic to where the name is calculated already. Do a else-if, for example.

Are you aware of the TAV/test-all-versions setup and how to run it?

API docs for Koa

@lukiano
Copy link
Contributor Author

lukiano commented May 4, 2022

I'll work on these changes today. Sorry for the delay.

@lukiano
Copy link
Contributor Author

lukiano commented May 5, 2022

I've addressed the feedback and ran tav

@rauno56
Copy link
Member

rauno56 commented Jun 22, 2022

Thanks and congrats about your first contribution, @lukiano!

@rauno56 rauno56 merged commit fa4fe9c into open-telemetry:main Jun 22, 2022
@dyladan dyladan mentioned this pull request Aug 8, 2022
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.

2 participants