Add Valkey Instrumentation#3478
Conversation
|
|
| @retryable | ||
| def check_redis_connection(): | ||
| connection = redis.Redis(host=REDIS_HOST, port=REDIS_PORT) | ||
| connection = valkey.Redis(host=REDIS_HOST, port=REDIS_PORT) |
There was a problem hiding this comment.
Thank you watching for this PR, sir
But, This is a draft of PR.
Could you waiting for ready.
There was a problem hiding this comment.
@xrmx
Thank you for your patience. Would you mind taking a look and reviewing this PR?
There was a problem hiding this comment.
this PR is so large that it is difficult to review. I suggest splitting it into several smaller PRs, as that would make more sense.
There was a problem hiding this comment.
Thanks for the feedback! This instrumentation PR is actually a near-one-for-one copy of the existing Redis instrumentation, with only the service-specific bits renamed and a handful of Valkey-specific tweaks. To see how small the diff really is, you can run:
git diff --no-index instrumentation/opentelemetry-instrumentation-redis/src \
instrumentation/opentelemetry-instrumentation-valkey/src
git diff --no-index instrumentation/opentelemetry-instrumentation-redis/tests/__init__.py \
instrumentation/opentelemetry-instrumentation-valkey/tests/__init__.py
git diff --no-index instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py \
instrumentation/opentelemetry-instrumentation-valkey/tests/test_valkey.py
git diff --no-index instrumentation/opentelemetry-instrumentation-redis/LICENCE \
instrumentation/opentelemetry-instrumentation-valkey/LICENCE
git diff --no-index instrumentation/opentelemetry-instrumentation-redis/pyproject.toml \
instrumentation/opentelemetry-instrumentation-valkey/pyproject.toml
git diff --no-index instrumentation/opentelemetry-instrumentation-redis/README.rst \
instrumentation/opentelemetry-instrumentation-valkey/README.rst
git diff --no-index instrumentation/opentelemetry-instrumentation-redis/test-requirements.txt \
instrumentation/opentelemetry-instrumentation-valkey/test-requirements.txt
git diff --no-index tests/opentelemetry-docker-tests/tests/redis/test_redis_functional.py \
tests/opentelemetry-docker-tests/tests/valkey/test_valkey_functional.pyBecause the core of this PR is simply “copy + rename + small adjustments,” I think a single PR still keeps reviewable granularity. Please let me know if you’d prefer me to split out any particular piece!
sightseeker
force-pushed
the
feature/valkey
branch
from
669c7f8 to
73d1eca
Compare
sightseeker
force-pushed
the
feature/valkey
branch
from
73d1eca to
24ecc80
Compare
sightseeker
changed the title
sightseeker
changed the title
There's no opentelemetry-instrumentation-valkey as of now, although it is WIP [1]. Fix the link for now, we can revert it later. [1] open-telemetry/opentelemetry-python-contrib#3478 Signed-off-by: Mikhail Koviazin <mikhail.koviazin@aiven.io>
; Conflicts: ; CHANGELOG.md
- Bump valkey requires-python to >=3.10 and remove 3.9 classifier/tox factor - Remove unreachable return statement after warning in uninstrument_client - Add redis-valkey-base to opentelemetry-contrib-instrumentations - Update bootstrap_gen with redis-valkey-base and bump valkey to 0.63b0.dev - Migrate redis/valkey docker functional tests to SERVER_ADDRESS/SERVER_PORT - Update valkey docker tests to use db.redis.* attributes per semconv decision
|
Thanks @tammy-baylis-swi! Done — dropped Python 3.9 from the valkey and redis-valkey-base packages ( |
; Conflicts: ; CHANGELOG.md ; tests/opentelemetry-docker-tests/tests/check_availability.py
; Conflicts: ; tests/opentelemetry-docker-tests/tests/test-requirements.txt
|
Thanks for the PR! Just a heads-up: we no longer update Please add the appropriate changelog fragment for this change instead of editing |
; Conflicts: ; instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py ; instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
|
Thanks for the heads-up about towncrier! I've added a changelog fragment at I also had to deal with a large merge conflict from the upstream Redis semconv stability migration (#4370). Integrating it into the shared base package would be a sizable refactor that significantly expands the scope of this PR, so for now I've kept Redis on the upstream version and the shared base package is only used by Valkey. I'd like to revisit consolidating Redis into the shared base in a follow-up PR after this one lands, does that work for you? |
Thank you, that was mine so I appreciate it 🙂
That works for me. Not sure what happened with the most recent merge but some calls need to be run again please:
|
Run tox -e generate and tox -e generate-workflows to: - Add redis-valkey-base entry to instrumentation/README.md - Regenerate test.yml and core_contrib_test.yml with valkey jobs - Update lint.yml with valkey lint env - Refresh uv.lock
|
@tammy-baylis-swi |
|
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. |
; Conflicts: ; opentelemetry-contrib-instrumentations/pyproject.toml ; opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py ; tests/opentelemetry-docker-tests/tests/test-requirements.txt
|
Hi @tammy-baylis-swi! I've resolved the latest merge conflicts with |
…headers Replace the full Apache 2.0 license boilerplate with the one-line SPDX identifier on all newly added Python files, following the repository-wide migration in open-telemetry#4533.
tammy-baylis-swi
left a comment
tammy-baylis-swi
left a comment
There was a problem hiding this comment.
Thank you! This lgtm.
The check-links will fail until the first readthedocs publish. PyPI publish destination was sorted a while ago. @sightseeker is on the component_owners list and has actively worked hard on this for a while.
More Approvers will also have to take a look.
tammy-baylis-swi
moved this from Approved PRs that need fixes
to Approved PRs
in Python PR digest
|
A big thank you to @tammy-baylis-swi for the approval 🙏 @xrmx, @fabiob, @MikeGoldsmith , @zhengkezhou1 Would any of you be able to take another look when you have a moment? I'm happy to clarify anything or make further adjustments. Thanks so much for your time! |
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| """ | ||
| Shared instrumentation base for Redis and Valkey. |
There was a problem hiding this comment.
I am very late to the discussion but wondering if instead we should not move this helpers in opentelemetry-instrumentation instead so that we can reuse them more generally. I'm not asking to move code just to start a discussion.
There was a problem hiding this comment.
Also if we are introducing some helpers they should be used by both instrumentations and not just by one.
|
|
||
| _BACKEND_NAME = "valkey" | ||
| _DB_SYSTEM = "valkey" | ||
| _DB_SYSTEM_ATTR = DB_SYSTEM |
There was a problem hiding this comment.
Looks like this is implementing the old semconv, I'm not sure we should introduce new instrumentations with outdated semconv if there's a stable one available. Redis instrumentation got updated in 0059b27
github-project-automation
Bot
moved this from Approved PRs
to Reviewed PRs that need fixes
in Python PR digest
Migrate the Valkey instrumentation (and the shared redis-valkey-base helpers) to support the new stable DB and HTTP semantic conventions via OTEL_SEMCONV_STABILITY_OPT_IN, mirroring what was done for Redis in open-telemetry#4370. - redis-valkey-base: switch helpers to use _semconv stability helpers (_set_db_system, _set_db_statement, _set_db_redis_database_index, _set_http_net_peer_name_client, _set_http_peer_port_client, _set_net_transport) and resolve schema_url via signal-type opt-in - valkey: drop hardcoded attribute keys from KVStoreConfig, mark package as semconv migration, switch tracer schema_url to the stability-aware helper - tests: cover default / database / database/dup / http / http/dup stability modes for db.system, db.statement and connection attrs
|
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. |
11 participants
Description
This PR introduces a new Valkey instrumentation plugin for the OpenTelemetry Python Contrib library.
users can now automatically capture and export Valkey trace data alongside existing OpenTelemetry spans. This fills the gap for applications leveraging Valkey’s Client library, enabling end-to-end distributed tracing without manual span management.
Notes:
valkey-searchmodule is not yet GA, related tests are intentionally omitted in this PR.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
instrumentation/opentelemetry-instrumentation-valkey/tests/covering:db.statement,db.valkey.args_length,db.valkey.database_index,net.peer.name)opentelemetry-instrumentation-valkey, verified spans in Jaeger backendtox -e py38-test-instrumentation-valkeytox -e py39-test-instrumentation-valkeytox -e py310-test-instrumentation-valkeytox -e py311-test-instrumentation-valkeytox -e py312-test-instrumentation-valkeytox -e py313-test-instrumentation-valkeytox -e pypy3-test-instrumentation-valkeytox -e lint-instrumentation-valkeyDoes This PR Require a Core Repo Change?
Checklist:
CHANGELOG.mdentry under “New Features”)