fix(grpc): Allow non-list interceptors #3520
Merged
Conversation
This file contains hidden or 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
The GRPC integration assumes that if interceptors are provided, they will be a list, when GRPC itself types them as Sequence. With this change, we're making the codepaths using interceptors more robust by explicitly turning them into lists before manipulating them.
xrmx
reviewed
May 21, 2025
instrumentation/opentelemetry-instrumentation-grpc/tests/test_aio_server_interceptor.py
Outdated
Show resolved
Hide resolved
xrmx
approved these changes
May 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
The OpenTelemetry GRPC instrumentation assumes that server interceptors will be provided as a list -- it's using the
.insert()method both in the sync and async server wrapper to add an interceptor of its own.GRPC however states
interceptorsis just aSequence(sync, async) and not necessarily alist, so we shouldn't assume that whatever gets passed in has aninsert()method.This originally surfaced in getsentry/sentry-python#4389. In case both Sentry and OTel are instrumenting GRPC, Sentry makes
interceptorsatuple, which then throws an exception in OTel since tuples don't have aninsert()method.For good measure, I also applied this fix to channel interceptors (in the
_add_interceptorswrapper).Type of change
How Has This Been Tested?
Added two test cases:
instrumentation/opentelemetry-instrumentation-grpc/tests/test_aio_server_interceptor.py(async server interceptors)instrumentation/opentelemetry-instrumentation-grpc/tests/test_server_interceptor.py(sync server interceptors)Undoing the changes in
instrumentation/grpc/__init__.pyand running the tests makes the new test cases fail.The
secure_channelandinsecure_channelfix doesn't have a corresponding test case. I can add one if needed.I also changed some of the existing tests to call
uninstrument()in afinallyif they callinstrument()at the start. Not all of them would uninstrument if there was an exception, which was causing weird issues for me when sometimes things in other tests wouldn't be patched correctly.Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.