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

uninstruemnt existing instances before uninstrumenting fastapi class #1258

Merged
merged 26 commits into from Sep 19, 2022

Conversation

TheAnshul756
Copy link
Contributor

@TheAnshul756 TheAnshul756 commented Sep 1, 2022

Description

Create a static list of all instances of InstrumentedFastAPI and fix uninstrument function.

Fixes #1256

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

  • tox -e test-instrumentation-fastapi

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

@TheAnshul756 TheAnshul756 requested a review from a team as a code owner September 1, 2022 08:38
@TheAnshul756 TheAnshul756 marked this pull request as draft September 1, 2022 08:41
@TheAnshul756 TheAnshul756 changed the title uninstruemnt existing instances before uninstrumenting fastapi class [Draft] uninstruemnt existing instances before uninstrumenting fastapi class Sep 1, 2022
@TheAnshul756 TheAnshul756 marked this pull request as ready for review September 6, 2022 06:09
@TheAnshul756 TheAnshul756 changed the title [Draft] uninstruemnt existing instances before uninstrumenting fastapi class uninstruemnt existing instances before uninstrumenting fastapi class Sep 6, 2022
@lzchen
Copy link
Contributor

lzchen commented Sep 13, 2022

@TheAnshul756 @srikanthccv

So, as discussed, we wanted to uninstrument existing instrumented apps when we call instrumentor.uninstrument()

When was this discussed? As a user this makes sense to me but we'll have to do this for other instrumentations as well. Could we make tracking issues for those?

@srikanthccv
Copy link
Member

This was briefly discussed in one of the SIG meetings when the topic was brought up by I believe Sanket. Some instrumentations haven't been fixed for this case but generally, we are trying to ensure the uninstrument works as expected. Sure, I will create an issue for the instrumentation we have missed this.

@sanketmehta28
Copy link
Member

Resolve the generate test issue

@srikanthccv srikanthccv enabled auto-merge (squash) September 15, 2022 18:55
@srikanthccv srikanthccv merged commit 2b7a005 into open-telemetry:main Sep 19, 2022
@TheAnshul756 TheAnshul756 deleted the fix-uninstrument-fastapi branch September 19, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uninstrument Existing fastapi instances
4 participants