-
Notifications
You must be signed in to change notification settings - Fork 533
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
chore(koa): migrate koa example to opentelemetry-instrumentation-koa #1118
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1118 +/- ##
==========================================
+ Coverage 96.07% 96.75% +0.68%
==========================================
Files 14 24 +10
Lines 892 1234 +342
Branches 191 270 +79
==========================================
+ Hits 857 1194 +337
- Misses 35 40 +5
|
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, left few minor comments.
The PR is marked as fix
which will trigger a release of a new patch
version for the instrumentation and will go into the changelog and release notes.
Since this PR only changes example files, it should be marked as chore
which will not affect the release :)
plugins/node/opentelemetry-instrumentation-koa/examples/package.json
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-koa/examples/package.json
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-koa/examples/package.json
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-koa/examples/package.json
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-koa/examples/package.json
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-koa/examples/src/tracer.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-koa/examples/src/client.ts
Outdated
Show resolved
Hide resolved
// Initialize the OpenTelemetry APIs to use the NodeTracerProvider bindings | ||
provider.register(); | ||
|
||
return api.trace.getTracer(serviceName); |
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.
tracer name is usually set to a package name ( as set in the name
field in package.json
) which in our case is koa-example
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.
Also here I took this behavior from here: https://github.com/open-telemetry/opentelemetry-js-contrib/pull/939/files, see tracer.ts file, line 50. this PR appears in the issue as a reference, so I thought this is ok they will be aligned.
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.
I think this is an overlooked mistake in express example PR.
This is a minor issue but would love other opinions @open-telemetry/javascript-approvers
plugins/node/opentelemetry-instrumentation-koa/examples/src/client.ts
Outdated
Show resolved
Hide resolved
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.
left a few last optional comments which are not blockers on my end.
Great work and thanks for taking this task and contributing :)
plugins/node/opentelemetry-instrumentation-koa/examples/src/server.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-koa/examples/src/server.ts
Outdated
Show resolved
Hide resolved
// Initialize the OpenTelemetry APIs to use the NodeTracerProvider bindings | ||
provider.register(); | ||
|
||
return api.trace.getTracer(serviceName); |
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.
I think this is an overlooked mistake in express example PR.
This is a minor issue but would love other opinions @open-telemetry/javascript-approvers
…ry-js-contrib into migrate-koa-examples
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.
Thank you for addressing everything! great job :)
LGTM
Which problem is this PR solving?
Short description of the changes
Checklist
npm run test-all-versions
for the edited package(s) on the latest commit if applicable.