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.

Sign up for GitHub

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

Jump to bottom

Add asyncclick instrumentation #3319

Merged
merged 5 commits into from
Mar 28, 2025
Merged

Add asyncclick instrumentation #3319

merged 5 commits into from
Mar 28, 2025

Conversation

jomcgi
Copy link
Contributor

@jomcgi jomcgi commented Mar 2, 2025 •
edited
Loading

Description

  • Add instrumentation for asyncclick based CLI apps
  • Add tox
  • Add Changelog
  • Update workflows

This implementation is based on the original click instrumentation work by:

Fixes #3318

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • tox -e test-instrumentation-asyncclick

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@jomcgi jomcgi force-pushed the feat-3318/asyncclick-autoinstrumentation branch from 874ba33 to 17b14da Compare March 2, 2025 22:03
@jomcgi jomcgi mentioned this pull request Mar 2, 2025
Open
@jomcgi jomcgi force-pushed the feat-3318/asyncclick-autoinstrumentation branch 5 times, most recently from a1b5c89 to ff0d7fd Compare March 2, 2025 23:36
@jomcgi jomcgi changed the title [WIP] Add asyncclick instrumentation Add asyncclick instrumentation Mar 2, 2025
@jomcgi jomcgi changed the title Add asyncclick instrumentation [WIP] Add asyncclick instrumentation Mar 3, 2025
@jomcgi jomcgi force-pushed the feat-3318/asyncclick-autoinstrumentation branch 5 times, most recently from 23d64e8 to bac1c1b Compare March 3, 2025 13:54
@github-actions github-actions bot requested a review from nemoshlag March 3, 2025 13:54
@jomcgi jomcgi force-pushed the feat-3318/asyncclick-autoinstrumentation branch from d0b2350 to ae61899 Compare March 5, 2025 21:52
@jomcgi jomcgi changed the title [WIP] Add asyncclick instrumentation Add asyncclick instrumentation Mar 5, 2025
@jomcgi jomcgi marked this pull request as ready for review March 5, 2025 22:15
@jomcgi jomcgi requested a review from a team as a code owner March 5, 2025 22:15
emdneto
emdneto reviewed Mar 11, 2025
View reviewed changes
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt add that to the pyright strict list in pyproject.toml?

@jomcgi jomcgi force-pushed the feat-3318/asyncclick-autoinstrumentation branch from ae61899 to f0759b7 Compare March 12, 2025 09:16
@jomcgi
Copy link
Contributor Author

jomcgi commented Mar 12, 2025

wdyt add that to the pyright strict list in pyproject.toml?

Done!

@jomcgi jomcgi force-pushed the feat-3318/asyncclick-autoinstrumentation branch from f0759b7 to 9d09175 Compare March 12, 2025 09:24
@jomcgi
Add asyncclick instrumentation
69d88e3 
* Add instrumentation for asyncclick based CLI apps
* Add tox
* Add Changelog
* Update workflows

This implementation is based on the original click instrumentation work by:
- Emídio Neto <9735060+emdneto@users.noreply.github.com>
- Anuraag (Rag) Agrawal <anuraaga@gmail.com>
- Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
@jomcgi jomcgi force-pushed the feat-3318/asyncclick-autoinstrumentation branch from 9d09175 to 69d88e3 Compare March 13, 2025 11:13
emdneto
emdneto reviewed Mar 26, 2025
View reviewed changes
emdneto
emdneto reviewed Mar 26, 2025
View reviewed changes
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work so far on this one. I'll share this in SIG tomorrow and see if we can at least create the package in PyPI while this doesn't get merged.

jomcgi and others added 2 commits March 27, 2025 09:00
@jomcgi @emdneto
Update CHANGELOG.md
74ba663 
Co-authored-by: Emídio Neto <9735060+emdneto@users.noreply.github.com>
@jomcgi
Merge branch 'main' into feat-3318/asyncclick-autoinstrumentation
a13e4b1
@jomcgi
Copy link
Contributor Author

jomcgi commented Mar 27, 2025

Thanks! I'll try to attend too.
I didn't know the best place to advocate for reviews / feedback so I really appreciate the help.

xrmx
xrmx approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the small nit in the example LGTM. Hopefully this is not used by long running servers and so we may never need to introduce code to disable the instrumentation in these cases.

@xrmx xrmx merged commit 0ea9998 into open-telemetry:main Mar 28, 2025
720 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xrmx xrmx xrmx approved these changes

@emdneto emdneto emdneto approved these changes

@nemoshlag nemoshlag Awaiting requested review from nemoshlag

Assignees

@nemoshlag nemoshlag

@jomcgi jomcgi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: automatic instrumentation for asyncclick
4 participants
@jomcgi @xrmx @emdneto @nemoshlag