-
Notifications
You must be signed in to change notification settings - Fork 622
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
Enable passing explicit urls to exclude in instrumentation in FastAPI #486
Merged
Merged
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
f728abb
feat: Enable passing explicit urls to exclude from instrumentation in…
cdvv7788 8c4eee0
docs: Update changelog (fastapi #486)
cdvv7788 17a0269
refactor: Rename _excluded_urls to _excluded_urls_from_env in fastapi…
cdvv7788 f047153
refactor: Move excluded urls logic to the _instrument function
cdvv7788 58c20cc
Merge branch 'main' into fastapi-excluded_urls
lzchen b81dbf5
refactor: Use ExcludeList only internally in the instrumentation(fast…
cdvv7788 eea46e4
Merge branch 'main' into fastapi-excluded_urls
cdvv7788 4ce59f2
Merge branch 'main' into fastapi-excluded_urls
lzchen 178643d
Merge branch 'main' into fastapi-excluded_urls
lzchen 314d329
Merge branch 'main' into fastapi-excluded_urls
ocelotl a335531
refactor: Move utils method into http-utils package
cdvv7788 34fa727
test: Remove unneeded variable in explicit excluded_urls test
cdvv7788 b659f08
fix: Resolve docs generation issue
cdvv7788 180031e
fix: Remove unneeded import
cdvv7788 7bce354
Merge branch 'main' into fastapi-excluded_urls
cdvv7788 59a504c
Merge branch 'main' into fastapi-excluded_urls
02fa810
fix: wrong variable name in fastapi instrumentation
cdvv7788 b495dfe
Merge branch 'main' into fastapi-excluded_urls
lzchen fbaa815
Merge branch 'main' into fastapi-excluded_urls
ocelotl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,13 +32,19 @@ class FastAPIInstrumentor(BaseInstrumentor): | |
_original_fastapi = None | ||
|
||
@staticmethod | ||
def instrument_app(app: fastapi.FastAPI, tracer_provider=None): | ||
"""Instrument an uninstrumented FastAPI application. | ||
""" | ||
def instrument_app( | ||
app: fastapi.FastAPI, | ||
tracer_provider=None, | ||
excluded_urls=None, | ||
): | ||
"""Instrument an uninstrumented FastAPI application.""" | ||
if not getattr(app, "is_instrumented_by_opentelemetry", False): | ||
if excluded_urls is None: | ||
excluded_urls = _excluded_urls | ||
|
||
app.add_middleware( | ||
OpenTelemetryMiddleware, | ||
excluded_urls=_excluded_urls, | ||
excluded_urls=excluded_urls, | ||
span_details_callback=_get_route_details, | ||
tracer_provider=tracer_provider, | ||
) | ||
|
@@ -47,6 +53,7 @@ def instrument_app(app: fastapi.FastAPI, tracer_provider=None): | |
def _instrument(self, **kwargs): | ||
self._original_fastapi = fastapi.FastAPI | ||
_InstrumentedFastAPI._tracer_provider = kwargs.get("tracer_provider") | ||
_InstrumentedFastAPI._excluded_urls = kwargs.get("excluded_urls") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since _INstrumentedFastAPI is private and not supposed to be used directly, can we move env vs kwargs logic here instead? |
||
fastapi.FastAPI = _InstrumentedFastAPI | ||
|
||
def _uninstrument(self, **kwargs): | ||
|
@@ -55,12 +62,18 @@ def _uninstrument(self, **kwargs): | |
|
||
class _InstrumentedFastAPI(fastapi.FastAPI): | ||
_tracer_provider = None | ||
_excluded_urls = None | ||
|
||
def __init__(self, *args, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
excluded_urls = ( | ||
_InstrumentedFastAPI._excluded_urls | ||
if _InstrumentedFastAPI._excluded_urls is not None | ||
else _excluded_urls | ||
) | ||
self.add_middleware( | ||
OpenTelemetryMiddleware, | ||
excluded_urls=_excluded_urls, | ||
excluded_urls=excluded_urls, | ||
span_details_callback=_get_route_details, | ||
tracer_provider=_InstrumentedFastAPI._tracer_provider, | ||
) | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why not allow users to pass in a sequence of strings instead? Basically what can be passed into the env var should be replicated here.
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.
No real reason, I can change it to work that way.
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.
I think it is preferred. Also users don't have to import
ExcludeList
either.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.
Done