-
Notifications
You must be signed in to change notification settings - Fork 822
feat(redis): support semantic convention opt-in and emit stable Redis attributes #3826
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
base: main
Are you sure you want to change the base?
Conversation
|
Unit tests coming soon; will update this PR accordingly. |
e82684e to
c98b9fe
Compare
|
Just realized this change is more of a feature enhancement than a bug fix. I have amended the last commit to use "feat(redis)" prefix and force-pushed the updated branch |
84e6cf7 to
c98b9fe
Compare
|
Hi All, My changes for Redis instrumentation's stable semantic conventions are working as expected and thoroughly tested locally. However, CI shows some test failures that I'd like to discuss. 1. Redis Async Tests (test_watch_error_async, test_watch_error_async_only_client)Issue: AssertionError: <StatusCode.ERROR: 2> != <StatusCode.UNSET: 0> Explanation: My updated code now correctly sets StatusCode.ERROR for general failed Redis commands and populates the error.type attribute. This aligns with OpenTelemetry semantic conventions, which state that error.type is "Conditionally Required If and only if the operation failed." However, WatchError in Redis signifies an aborted transaction, not an execution failure of individual commands. The tests' FakeRedis seems to cause WatchError exceptions for individual SET commands. My instrumentation is currently catching these as general Exception and setting StatusCode.ERROR, but the tests still expect StatusCode.UNSET. Proposed: I am thinking about updating the instrumentation code to specifically handle redis.WatchError by setting the span's StatusCode to UNSET (as an aborted operation) and re-raising the exception. This aligns with how WatchError is already handled for pipeline spans and ensures semantic correctness, allowing the current test assertions to pass. Any thoughts on that? pypy3-test-util-genai Test RunIssue: ImportError: No module named 'opentelemetry.util.genai.handler' Explanation: This is a tox.ini configuration issue; the opentelemetry-util-genai package isn't being correctly installed in the pypy3 environment. I have a local fix for the tox.ini (to explicitly install the package using -e {toxinidir}/util/opentelemetry-util-genai), which made this test pass locally. Proposed: Should I go ahead and include this tox.ini update as part of this PR, or would you prefer it in a separate change? I'm ready to implement these fixes based on your guidance. Thanks! cc: @pmcollins Update: Following some research into the semantic meaning of redis.WatchError, I have already implemented the proposed change. The instrumentation now specifically handles redis.WatchError by setting the span's StatusCode to UNSET (as an aborted operation) and re-raising the exception. This aligns with how WatchError is already handled for pipeline spans and ensures semantic correctness. Also, all tests are passed locally. Update 2: Looks like pypy3-test-util-genai issue is gone. Please disregard. |
… and script attributes (open-telemetry#3826)
… and script attributes (open-telemetry#3826)
d6313cb to
2c36ebc
Compare
… and script attributes (open-telemetry#3826)
2c36ebc to
420b76c
Compare
265b57b to
9f16358
Compare
|
I added unit tests for the Redis semantic convention stability feature, verifying that span attributes are correctly emitted based on the OTEL_SEMCONV_STABILITY_OPT_IN environment variable. To ensure reliable and isolated testing, the tests are structured to run in separate subprocesses. This approach is necessary to overcome Python's module caching, which would otherwise cause test pollution, as the environment variable is read only once at import time. A main test file (test_semconv.py) now launches a helper script (_test_semconv_helper.py) for each mode (default, database, and database/dup), with each run validating attributes for single commands, pipelines, and error scenarios. |
9f16358 to
e20cea6
Compare
|
Hi Team, may I request your review and comments here? Thank you! |
|
|
||
|
|
||
| # Helper function to set all common span attributes | ||
| def _set_span_attributes( |
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.
There are some helpers in util.py that look like they also collect values and set attributes. Would any of them help here, to reduce possible duplication?
| @@ -0,0 +1,42 @@ | |||
| import os | |||
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.
Nit: let's add the OTel header to this and the helper
| import os | |
| # Copyright The OpenTelemetry Authors | |
| # | |
| # Licensed under the Apache License, Version 2.0 (the "License"); | |
| # you may not use this file except in compliance with the License. | |
| # You may obtain a copy of the License at | |
| # | |
| # http://www.apache.org/licenses/LICENSE-2.0 | |
| # | |
| # Unless required by applicable law or agreed to in writing, software | |
| # distributed under the License is distributed on an "AS IS" BASIS, | |
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| # See the License for the specific language governing permissions and | |
| # limitations under the License. | |
| # | |
| import os |
|
Thank you for taking the time to review this, @tammy-baylis-swi ! I’m planning to resume working on this PR. However, I just realized that Semantic conventions for Redis client operations are still marked as "Development." Would it be better to wait until they reach "Stable" status before moving forward? |
Awesome @luke6Lh43 ! No need to wait. 👍 The |
Description
This PR updates the Redis instrumentation to support semantic convention opt-in, aligning the implementation with the pattern used in the Requests instrumentation (where stable semantic conventions were already implemented before). Depending on the value of the OTEL_SEMCONV_STABILITY_OPT_IN environment variable, the instrumentation will emit either the legacy (experimental) or stable Redis span attributes, or both (database/dup).
Fixes #2885 #2930
Type of change
How Has This Been Tested?
Test 1: Default behavior - no environment variable set
Test 2: OTEL_SEMCONV_STABILITY_OPT_IN set to "database"
Test 3: OTEL_SEMCONV_STABILITY_OPT_IN set to "database/dup"
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.