starlette: add missing configuration params from ASGI Middleware#4566
starlette: add missing configuration params from ASGI Middleware#4566Fiery-Fenix wants to merge 9 commits into
Conversation
|
Thanks for the PR! Just a heads-up: we no longer update Please add the appropriate changelog fragment for this change instead of editing |
Fiery-Fenix
force-pushed
the
feat/strarlette_exclude_spans
branch
from
47090ed to
d9081d4
Compare
Fiery-Fenix
force-pushed
the
feat/strarlette_exclude_spans
branch
from
d9081d4 to
6a36ed4
Compare
Thanks for note! I have updated PR with correct changelog fragment |
tammy-baylis-swi
left a comment
tammy-baylis-swi
left a comment
There was a problem hiding this comment.
This lgtm!
The typecheck failure is coming from genAI instrumentation, not this code. Continuing to use schema url for 1.11.0 makes sense as Starlette instrumentation hasn't yet migrated to new semconv. I appreciate the tests.
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
| ) | ||
| self._is_instrumented_by_opentelemetry = True | ||
| # adding apps to set for uninstrumenting | ||
| _InstrumentedStarlette._instrumented_starlette_apps.add(self) |
There was a problem hiding this comment.
Since we delegate to instrument_app now, this might be a double add?
There was a problem hiding this comment.
Yes, you're right - it's a double add, removed.
| return _instruments | ||
|
|
||
| def _instrument(self, **kwargs: Unpack[InstrumentKwargs]): | ||
| def _instrument(self, **kwargs: Any): |
There was a problem hiding this comment.
Because it's LSP (Liskov Substitution Principle) violation, we shouldn't break a contract defined by a base class.
BaseInstrumentor._instrument defines **kwargs as Any, so I'm simply keeping the contract of the BaseInstrumentor._instrument.
Also, I have checked other instrumentation libraries and found that only 3 out of 49 libraries are using Unpack typing, so I'm assuming that it's not a common approach to use it.
There was a problem hiding this comment.
Also, I have checked other instrumentation libraries and found that only 3 out of 49 libraries are using Unpack typing, so I'm assuming that it's not a common approach to use it.
Yes, I added them.
Because it's LSP (Liskov Substitution Principle) violation, we shouldn't break a contract defined by a base class.
Yes, I know, but fixing the _instrument is more work, and it's easier to start making the packages better looking before doing that.
There was a problem hiding this comment.
I see, sound reasonable, I have returned Unpack part and updated it to reflect arguments changes in this PR
5 participants
Description
Unlike FastAPI instrumentation, Starlette manual instrumentation is missing several configuration parameters available in underlying ASGI
OpenTelemetryMiddleware.This PR is adding passthrough of the following
OpenTelemetryMiddlewareparameters to the Starletteinstrument_appfunction:excluded_urlshttp_capture_headers_server_requesthttp_capture_headers_server_responsehttp_capture_headers_sanitize_fieldsexclude_spansChanges in this PR are mostly port of respective functionality from FastAPI inbstrumentation.
Additional tests were added to high-level coverage of the new configuration parameters.
Partially fixing #3725, at least for manual instrumentation
Type of change
How Has This Been Tested?
Tests has passed for all supported platforms:
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.