-
Notifications
You must be signed in to change notification settings - Fork 544
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(cucumber): add instrumentation for @cucumber/cucumber #1252
Conversation
d1af88f
to
b57949d
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1252 +/- ##
==========================================
- Coverage 96.06% 95.74% -0.32%
==========================================
Files 14 17 +3
Lines 914 1128 +214
Branches 199 230 +31
==========================================
+ Hits 878 1080 +202
- Misses 36 48 +12
|
3a4beed
to
2997d73
Compare
c98dc4a
to
c824a95
Compare
2e7c62e
to
07f31bf
Compare
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 your contribution.
Always happy to see new instrumentation :)
I didn't finish the review yet but added a few comments in the meanwhile if you want to get some quick feedback.
Few general requests:
- Are you interested to list yourself as the component owner for this instrumentation? It means you are responsible to do reviews and fixes for this code in the future. If so, please add your github name to the component_owners.yml file
- CI currently fails because this instrumentation need to be added to release please configurations, you need to add it to release-please-config.json and .release-please-manifest.json (@dyladan - it needs to be added to both files, right?)
- The directory name
opentelemetry-instrumentation-x
is deprecated and we tend to add new instrumentation to a directory namedinstrumentation-x
. I think it will be best if you can rename the folder toinstrumentation-cucumber
instead ofopentelemetry-instrumentation-cucumber
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
Thanks for addressing my comments. Thanks again 🥇 |
Co-authored-by: Amir Blum <[email protected]>
@dyladan, resolved the conflicts. Everything looks green. :) |
🎉 nice! |
Which problem is this PR solving?
Adds a new instrumentation for cucumber-js that generates spans for its scenarios.
Short description of the changes
New package
@opentelemetry/instrumentation-cucumber
.