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(cucumber): add instrumentation for @cucumber/cucumber #1252

Merged
merged 23 commits into from
Jul 18, 2023

Conversation

Ugzuzg
Copy link
Contributor

@Ugzuzg Ugzuzg commented Oct 25, 2022

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.

@Ugzuzg Ugzuzg force-pushed the feat/cucumber branch 2 times, most recently from d1af88f to b57949d Compare October 25, 2022 17:58
@github-actions github-actions bot requested a review from obecny October 25, 2022 17:58
@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #1252 (b04d9ef) into main (b5fc0c4) will decrease coverage by 0.32%.
The diff coverage is 92.77%.

❗ Current head b04d9ef differs from pull request most recent head f25b2d2. Consider uploading reports for the commit f25b2d2 to get more accurate results

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     
Impacted Files Coverage Δ
...de/instrumentation-cucumber/src/instrumentation.ts 92.30% <92.30%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 98.24% <100.00%> (ø)
plugins/node/instrumentation-cucumber/src/types.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@Ugzuzg Ugzuzg force-pushed the feat/cucumber branch 3 times, most recently from 3a4beed to 2997d73 Compare November 2, 2022 15:58
@Ugzuzg Ugzuzg force-pushed the feat/cucumber branch 8 times, most recently from c98dc4a to c824a95 Compare November 3, 2022 21:17
@Ugzuzg Ugzuzg marked this pull request as ready for review November 3, 2022 21:20
@Ugzuzg Ugzuzg requested a review from a team November 3, 2022 21:20
@Ugzuzg Ugzuzg force-pushed the feat/cucumber branch 5 times, most recently from 2e7c62e to 07f31bf Compare November 6, 2022 10:51
Copy link
Member

@blumamir blumamir left a 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 named instrumentation-x. I think it will be best if you can rename the folder to instrumentation-cucumber instead of opentelemetry-instrumentation-cucumber

@blumamir
Copy link
Member

blumamir commented Nov 10, 2022

Thanks for addressing my comments.
I still plan to understand the cucumber package itself and do another review also for the patching logic. Approving for now so it is not blocked, but maybe I'll add few more questions and comments in the upcoming days :)

Thanks again 🥇

@Ugzuzg
Copy link
Contributor Author

Ugzuzg commented Jul 18, 2023

@dyladan, resolved the conflicts. Everything looks green. :)

@dyladan dyladan merged commit 82267ab into open-telemetry:main Jul 18, 2023
@Ugzuzg Ugzuzg deleted the feat/cucumber branch July 18, 2023 15:52
@dyladan dyladan mentioned this pull request Jul 17, 2023
@hermanbanken
Copy link
Contributor

hermanbanken commented Jul 24, 2023

🎉 nice! And cutting a release @dyladan / @Ugzuzg, would that be possible? see it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants