-
Notifications
You must be signed in to change notification settings - Fork 628
feat(cucumber): add instrumentation for @cucumber/cucumber #1252
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
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
blumamir
left a comment
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-xis 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-cucumberinstead ofopentelemetry-instrumentation-cucumber
plugins/node/opentelemetry-instrumentation-cucumber/src/instrumentation.ts
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.