-
Notifications
You must be signed in to change notification settings - Fork 10
feat(postgres): Implement Database Instrumentation Suppression #1127
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?
Changes from all commits
dcacbbc
d020fa5
48f47e4
b8ee39d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| import json | ||
| import logging | ||
| import time | ||
| from collections.abc import Iterator | ||
| from contextlib import contextmanager | ||
| from contextvars import ContextVar | ||
|
|
@@ -24,10 +23,37 @@ | |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Postgres SQLSTATE codes surfaced as named Sentry tags; others pass through as the raw code. | ||
| NOTABLE_SQLSTATES: dict[str, str] = { | ||
| "40P01": "deadlock_detected", | ||
| "57014": "query_canceled", # statement timeout | ||
| "40001": "serialization_failure", | ||
| "55P03": "lock_not_available", # lock timeout | ||
| "08006": "connection_failure", | ||
| "08003": "connection_does_not_exist", | ||
| "53300": "too_many_connections", | ||
| } | ||
|
|
||
| _log_context_var: ContextVar[dict[str, str] | None] = ContextVar( | ||
| "kaapi_log_context", default=None | ||
| ) | ||
|
|
||
| # OTel instrumentation scope emitted by SQLAlchemyInstrumentor; used to filter its spans. | ||
| _SQLALCHEMY_SCOPE = "opentelemetry.instrumentation.sqlalchemy" | ||
|
|
||
| # When True in the current context, SQLAlchemy DB spans are dropped before reaching Sentry. | ||
| _suppress_db_spans_var: ContextVar[bool] = ContextVar( | ||
| "kaapi_suppress_db_spans", default=False | ||
| ) | ||
|
|
||
|
|
||
| def _should_drop_db_span(otel_span: object) -> bool: | ||
| """True when DB-span suppression is active and `otel_span` is a SQLAlchemy span.""" | ||
| if not _suppress_db_spans_var.get(): | ||
| return False | ||
| scope = getattr(otel_span, "instrumentation_scope", None) | ||
| return scope is not None and getattr(scope, "name", None) == _SQLALCHEMY_SCOPE | ||
|
Comment on lines
+50
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add narrow type annotations to the new callables. Annotate 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
|
|
||
| def _emit_sentry_metric( | ||
| metric_type: str, | ||
|
|
@@ -194,7 +220,20 @@ def setup_telemetry(service_name: str | None = None) -> None: | |
| if settings.SENTRY_DSN: | ||
| from sentry_sdk.integrations.opentelemetry import SentrySpanProcessor | ||
|
|
||
| tracer_provider.add_span_processor(SentrySpanProcessor()) | ||
| class _DbSpanFilteringProcessor(SentrySpanProcessor): | ||
| """Drop SQLAlchemy DB spans from Sentry while suppress_db_instrumentation() is active. | ||
|
|
||
| Filters by instrumentation scope so only DB spans are skipped — HTTP/Requests | ||
| spans keep flowing. Skipping on_start leaves the span unmapped, and the parent | ||
| on_end safely no-ops on unmapped spans. | ||
| """ | ||
|
|
||
| def on_start(self, otel_span, parent_context=None): # type: ignore[override] | ||
| if _should_drop_db_span(otel_span): | ||
| return | ||
| super().on_start(otel_span, parent_context) | ||
|
|
||
| tracer_provider.add_span_processor(_DbSpanFilteringProcessor()) | ||
|
|
||
| trace.set_tracer_provider(tracer_provider) | ||
|
|
||
|
|
@@ -358,30 +397,56 @@ def suppress_http_instrumentation() -> Iterator[None]: | |
| otel_context.detach(token) | ||
|
|
||
|
|
||
| def record_db_query_finished( | ||
| @contextmanager | ||
| def suppress_db_instrumentation() -> Iterator[None]: | ||
| """Drop SQLAlchemy DB spans from the Sentry trace for the wrapped block. | ||
|
|
||
| Wrap LLM job execution so its DB reads/writes do not clutter the LLM waterfall. | ||
| Only DB spans are filtered (by _DbSpanFilteringProcessor via instrumentation | ||
| scope) — HTTP/Requests instrumentation stays active. Trade-off: the wrapped | ||
| DB queries also drop from the Sentry Queries page. | ||
| """ | ||
| token = _suppress_db_spans_var.set(True) | ||
| try: | ||
| yield | ||
| finally: | ||
| _suppress_db_spans_var.reset(token) | ||
|
|
||
|
|
||
| def record_db_query_failed( | ||
| *, | ||
| duration_ms: float, | ||
| operation: str | None = None, | ||
| error: bool = False, | ||
| sqlstate: str | None = None, | ||
| ) -> None: | ||
| """Emit DB query metrics to Sentry.""" | ||
| """Emit a DB query-failure counter to Sentry. Per-query duration/throughput come from spans.""" | ||
| if not settings.OTEL_ENABLED: | ||
| return | ||
|
|
||
| attrs: dict[str, str] = {} | ||
| attrs: dict[str, str | int | float] = {} | ||
| if operation: | ||
| attrs["db.operation"] = operation | ||
| if sqlstate: | ||
| attrs["db.sqlstate"] = sqlstate | ||
|
|
||
| _emit_sentry_metric("count", "db.query.total", 1, attributes=attrs) | ||
| _emit_sentry_metric( | ||
| "distribution", | ||
| "db.query.duration", | ||
| duration_ms, | ||
| unit="millisecond", | ||
| attributes=attrs, | ||
| ) | ||
| if error: | ||
| _emit_sentry_metric("count", "db.query.failed", 1, attributes=attrs) | ||
| _emit_sentry_metric("count", "db.query.failed", 1, attributes=attrs) | ||
|
|
||
|
|
||
| def _tag_db_error(sqlstate: str | None) -> None: | ||
| """Tag the active Sentry scope + span with a DB error's Postgres SQLSTATE.""" | ||
| if not sqlstate: | ||
| return | ||
| try: | ||
| span = trace.get_current_span() | ||
| if span.is_recording(): | ||
| span.set_attribute("db.sqlstate", sqlstate) | ||
| if sentry_sdk.get_client().is_active(): | ||
| sentry_sdk.set_tag("db.system", "postgresql") | ||
| sentry_sdk.set_tag("db.sqlstate", sqlstate) | ||
| name = NOTABLE_SQLSTATES.get(sqlstate) | ||
| if name: | ||
| sentry_sdk.set_tag("db.error.name", name) | ||
| except Exception: | ||
| logger.debug("[_tag_db_error] Failed to tag DB error | sqlstate: %s", sqlstate) | ||
|
|
||
|
|
||
| def record_db_pool_stats( | ||
|
|
@@ -523,6 +588,17 @@ def instrument_db_engine(engine: object) -> None: | |
| if getattr(engine, "_kaapi_db_telemetry_instrumented", False): | ||
| return | ||
|
|
||
| # DB query spans -> SentrySpanProcessor -> Sentry Insights/Queries. ProxyTracer defers | ||
| # to the real provider set later in setup_telemetry(), so load order here is safe. | ||
| try: | ||
| from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor | ||
|
|
||
| SQLAlchemyInstrumentor().instrument(engine=engine) | ||
| except Exception: | ||
| logger.exception( | ||
| "[instrument_db_engine] Failed to load SQLAlchemy span instrumentation" | ||
| ) | ||
|
|
||
| try: | ||
| from sqlalchemy import event | ||
| except Exception: | ||
|
|
@@ -551,7 +627,7 @@ def _before_cursor_execute( | |
| conn, cursor, statement, parameters, context, executemany | ||
| ) -> None: | ||
| del cursor, parameters, executemany | ||
| context._kaapi_db_started_at = time.perf_counter() | ||
| # Operation kept for error attribution; per-query timing now lives on the span. | ||
| context._kaapi_db_operation = ( | ||
| str(statement).split(None, 1)[0].upper() if statement else "UNKNOWN" | ||
| ) | ||
|
|
@@ -561,30 +637,20 @@ def _before_cursor_execute( | |
| def _after_cursor_execute( | ||
| conn, cursor, statement, parameters, context, executemany | ||
| ) -> None: | ||
| del cursor, statement, parameters, executemany | ||
| started_at = getattr(context, "_kaapi_db_started_at", None) | ||
| duration_ms = ( | ||
| (time.perf_counter() - started_at) * 1000 if started_at is not None else 0.0 | ||
| ) | ||
| operation = getattr(context, "_kaapi_db_operation", None) | ||
| record_db_query_finished( | ||
| duration_ms=duration_ms, operation=operation, error=False | ||
| ) | ||
| del cursor, statement, parameters, context, executemany | ||
| _emit_pool_metrics(conn.engine.pool) | ||
|
|
||
| @event.listens_for(engine, "handle_error") | ||
| def _handle_error(exception_context) -> None: | ||
| context = exception_context.execution_context | ||
| if context is None: | ||
| return | ||
| started_at = getattr(context, "_kaapi_db_started_at", None) | ||
| duration_ms = ( | ||
| (time.perf_counter() - started_at) * 1000 if started_at is not None else 0.0 | ||
| ) | ||
| operation = getattr(context, "_kaapi_db_operation", None) | ||
| record_db_query_finished( | ||
| duration_ms=duration_ms, operation=operation, error=True | ||
| operation = ( | ||
| getattr(context, "_kaapi_db_operation", None) | ||
| if context is not None | ||
| else None | ||
| ) | ||
| sqlstate = getattr(exception_context.original_exception, "sqlstate", None) | ||
| _tag_db_error(sqlstate) | ||
| record_db_query_failed(operation=operation, sqlstate=sqlstate) | ||
|
Comment on lines
643
to
+653
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- candidate file context ---'
sed -n '580,645p' backend/app/core/telemetry.py
printf '%s\n' '--- SQLAlchemy dependency declarations ---'
rg -n -i 'sqlalchemy|sqlmodel' pyproject.toml poetry.lock requirements*.txt setup.cfg setup.py 2>/dev/null || true
printf '%s\n' '--- related annotations and handlers ---'
rg -n 'handle_error|ExceptionContext|exception_context|event\.listens_for' backend/app pyproject.toml 2>/dev/null | head -120Repository: ProjectTech4DevAI/kaapi-backend Length of output: 6822 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- telemetry imports and function scope ---'
sed -n '1,90p' backend/app/core/telemetry.py
sed -n '520,640p' backend/app/core/telemetry.py
printf '%s\n' '--- dependency files ---'
git ls-files | rg '(^|/)(pyproject\.toml|poetry\.lock|requirements[^/]*\.txt|uv\.lock|Pipfile|setup\.cfg|setup\.py)$' || true
printf '%s\n' '--- SQLAlchemy references in tracked files ---'
rg -n -i 'sqlalchemy|ExceptionContext' --glob '!backend/app/core/telemetry.py' --glob '*.py' --glob '*.toml' --glob '*.txt' . | head -160Repository: ProjectTech4DevAI/kaapi-backend Length of output: 23558 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- declared and locked SQLAlchemy versions ---'
rg -n -A8 -B3 '^(name = "sqlalchemy"|name = "opentelemetry-instrumentation-sqlalchemy")' backend/uv.lock
printf '%s\n' '--- SQLAlchemy ExceptionContext references in local metadata or caches ---'
find . -path '*/site-packages/sqlalchemy*' -o -path '*/.venv/*sqlalchemy*' 2>/dev/null | head -40 || trueRepository: ProjectTech4DevAI/kaapi-backend Length of output: 1475 🌐 Web query:
💡 Result: To use the ExceptionContext object within a handle_error event handler in SQLAlchemy, you should import it from sqlalchemy.engine or directly from the sqlalchemy namespace [1][2]. The handle_error event is part of the DialectEvents class (in SQLAlchemy 2.0+), though it is registered using the Engine as the target [3][4]. The event handler receives a single argument, which is an instance of ExceptionContext [3][5]. You can import ExceptionContext using either of the following patterns: from sqlalchemy.engine import ExceptionContext # OR from sqlalchemy import ExceptionContext Key details regarding the handle_error event and ExceptionContext: 1. Event Location: As of SQLAlchemy 2.0, handle_error was moved from ConnectionEvents to DialectEvents to support connection pool pre-ping operations [3][6]. 2. Usage: ExceptionContext provides detailed information about an exception occurring within the scope of a database operation, such as the original exception, the statement executed, parameters, and flags like is_disconnect [3][7]. 3. Customization: Handlers can use ExceptionContext to modify how errors are handled, such as indicating that a connection should be invalidated, rewriting the exception, or logging details [3][5]. When using this hook, note that in certain contexts, such as connection pool pre-ping operations, the ExceptionContext.engine and ExceptionContext.connection attributes may be None, while the ExceptionContext.dialect attribute will remain available [3][4]. Citations:
Add a narrow annotation to Annotate 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| @event.listens_for(engine.pool, "checkout") | ||
| def _on_checkout(dbapi_connection, connection_record, connection_proxy) -> None: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| """Tests for the LLM/response Celery task wrappers in tasks/job_execution.py. | ||
|
|
||
| The service entrypoints are mocked; no DB, OTel provider, or broker is used. | ||
| Celery tasks are callable and run synchronously when invoked directly (self is | ||
| bound), so we drive the real wrapper — including the gevent_timeout decorator | ||
| and suppress_db_instrumentation — end to end. | ||
| """ | ||
|
|
||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
|
|
||
| from app.celery.tasks import job_execution | ||
| from app.core import telemetry | ||
|
|
||
| SENTINEL = object() | ||
|
|
||
|
|
||
| def _capture_suppression(captured: dict): | ||
| """execute_* stand-in that records whether DB spans are suppressed at call time.""" | ||
|
|
||
| def _exec(**kwargs): | ||
| captured["suppressed"] = telemetry._suppress_db_spans_var.get() | ||
| return SENTINEL | ||
|
|
||
| return _exec | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("task_name", "service_target"), | ||
| [ | ||
| ("run_llm_job", "app.services.llm.jobs.execute_job"), | ||
| ("run_llm_chain_job", "app.services.llm.jobs.execute_chain_job"), | ||
| ("run_response_job", "app.services.response.jobs.execute_job"), | ||
| ], | ||
| ) | ||
| class TestJobWrappers: | ||
| def test_returns_service_result(self, task_name, service_target): | ||
| task = getattr(job_execution, task_name) | ||
| captured: dict = {} | ||
| with ( | ||
| patch.object(job_execution, "_set_trace", lambda trace_id: None), | ||
| patch(service_target, _capture_suppression(captured)), | ||
| ): | ||
| result = task(project_id=1, job_id="job-1", trace_id="trace-1") | ||
|
|
||
| assert result is SENTINEL | ||
|
|
||
| def test_db_spans_suppressed_during_execution(self, task_name, service_target): | ||
| task = getattr(job_execution, task_name) | ||
| captured: dict = {} | ||
| with ( | ||
| patch.object(job_execution, "_set_trace", lambda trace_id: None), | ||
| patch(service_target, _capture_suppression(captured)), | ||
| ): | ||
| task(project_id=1, job_id="job-1", trace_id="trace-1") | ||
|
|
||
| assert captured["suppressed"] is True | ||
|
|
||
| def test_suppression_resets_after_return(self, task_name, service_target): | ||
| task = getattr(job_execution, task_name) | ||
| captured: dict = {} | ||
| with ( | ||
| patch.object(job_execution, "_set_trace", lambda trace_id: None), | ||
| patch(service_target, _capture_suppression(captured)), | ||
| ): | ||
| task(project_id=1, job_id="job-1", trace_id="trace-1") | ||
|
|
||
| assert telemetry._suppress_db_spans_var.get() is False |
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.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 39470
🏁 Script executed:
Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 15939
🏁 Script executed:
Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 11734
🏁 Script executed:
Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 2534
🌐 Web query:
OpenTelemetry Python Contrib v0.62b0 HTTPX instrumentation is_instrumentation_enabled suppress_instrumentation source💡 Result:
In the OpenTelemetry Python ecosystem, including version 0.62b0 of the httpx instrumentation, the functions
is_instrumentation_enabled()andis_http_instrumentation_enabled()are used to respect context-based suppression of telemetry [1]. These utilities are provided by theopentelemetry-instrumentationpackage [1][2]. The functionis_http_instrumentation_enabled()checks the current execution context for specific suppression keys [1]. It returnsTrueonly if neither generic instrumentation suppression nor HTTP-specific instrumentation suppression is active [1]. Specifically, it verifies that thesuppress_instrumentation(or the internal constant_SUPPRESS_INSTRUMENTATION_KEY) and_SUPPRESS_HTTP_INSTRUMENTATION_KEYare not set in the context [1]. Thehttpxinstrumentation makes use of these utilities to determine whether it should proceed with creating spans [3]. For example, in the internal request handling logic, the instrumentation callsis_http_instrumentation_enabled()before extracting parameters or creating spans [3]. If it returnsFalse, the instrumentation bypasses its tracing logic and immediately calls the underlying transport's request method [3]. Developers can programmatically suppress instrumentation within a block of code using context managers provided by the same utility module: -suppress_instrumentation(): Suppresses all instrumentation [1][2]. -suppress_http_instrumentation(): Suppresses only HTTP-specific instrumentation [1][2]. These context managers work by attaching a booleanTruevalue to the respective keys in thecontextobject, which the instrumentation check functions subsequently detect [1]. This mechanism is widely used across theopentelemetry-python-contribrepository to prevent recursive tracing or unwanted instrumentation in specific scenarios [4][1].Citations:
🌐 Web query:
OpenTelemetry Python Contrib v0.62b0 Requests instrumentation is_instrumentation_enabled source💡 Result:
In OpenTelemetry Python Contrib v0.62b0, the
is_http_instrumentation_enabledfunction—which is used by therequestsinstrumentation to determine whether to proceed with creating spans—is defined inopentelemetry.instrumentation.utils[1][2][3]. This utility function evaluates whether instrumentation is enabled globally and whether HTTP-specific instrumentation has been suppressed [1][3]. Specifically, the source code implementation is [1]: def is_http_instrumentation_enabled -> bool: return is_instrumentation_enabled and not context.get_value( _SUPPRESS_HTTP_INSTRUMENTATION_KEY) Whereis_instrumentation_enabled()checks for the absence of suppression keys in the current context [1]: def is_instrumentation_enabled -> bool: return not ( context.get_value(_SUPPRESS_INSTRUMENTATION_KEY) or context.get_value(_SUPPRESS_INSTRUMENTATION_KEY_PLAIN)) Therequestsinstrumentation utilizes this check within its instrumentedSession.sendmethod to decide whether to skip instrumentation for a given request [2]. Ifis_http_instrumentation_enabled()returnsFalse, the instrumentation performs no further tracing actions for that request [2].Citations:
Scope database suppression to SQLAlchemy spans.
suppress_db_instrumentation()sets the generic_SUPPRESS_INSTRUMENTATION_KEY. HTTPX and Requests also honor this key. Since each context wraps the full job execution, auto-instrumented HTTP requests in LLM, chain, and response jobs lose their spans. Use a SQLAlchemy-specific or narrowly scoped suppression mechanism in all three wrappers.📍 Affects 1 file
backend/app/celery/tasks/job_execution.py#L86-L98(this comment)backend/app/celery/tasks/job_execution.py#L107-L117backend/app/celery/tasks/job_execution.py#L126-L136🤖 Prompt for AI Agents