Repository navigation
chore: drop the asyncpg OpenTelemetry instrumentor - #59
Merged
Merged
Conversation
Keep SQLAlchemyInstrumentor as the only database instrumentor and add a test that DB queries still produce SQLAlchemy spans.
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
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.
Summary
The app ran two database instrumentors, so every query was traced twice. This keeps
SQLAlchemyInstrumentorand removesAsyncPGInstrumentor(capture_parameters=True)and its package.With one instrumentor left, nothing else would notice if it stopped producing spans. A new test,
tests/test_tracing.py, turns tracing on, makes a request, and checks that the SQLAlchemy instrumentor records a SELECT span under the request span.Why
Measured against Postgres 17 in Docker on the same host, with a new session per request and 100 concurrent workers:
The extra asyncpg spans were BEGIN, ROLLBACK and the pool pre-ping queries.
capture_parameters=Truealso copied query parameter values, user data included, into span attributes.What we lose
Watch out
opentelemetry-instrumentation-sqlalchemy(0.65b0, and 0.66b1 too) declaressqlalchemy < 2.1. On SQLAlchemy 2.1 it skips instrumentation, and lite-bootstrap silences the log line that reports the conflict. Keep thesqlalchemy<2.1pin until a release ships 2.1 support (open-telemetry/opentelemetry-python-contrib#5120). The new test fails in that case.Test plan
just lintand the full test suite pass, with coverage at 100%.SQLAlchemyInstrumentorremoved frombuild_app, the new test fails.