You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2026/05/25 21:59:39 UTC
[PR] Isolate Sentry instrumentation from task run [airflow]
potiuk opened a new pull request, #67505:
URL: https://github.com/apache/airflow/pull/67505
In `ConfiguredSentry.enrich_errors`, `prepare_to_enrich_errors`, `add_tagging` and `add_breadcrumbs` ran either outside any try block (line 142, pre-fix) or inside the same try-block as the wrapped `run(...)`. An exception raised by any of them propagated through the wrapper, so:
- A failure in non-critical Sentry instrumentation was indistinguishable from a real task failure.
- The wrapped `run` never executed — instrumentation could effectively prevent the task from running.
Reported as F-006 in the [`apache/tooling-agents` L3 task-sdk sweep `0920c77`](https://github.com/apache/tooling-agents/issues/24).
## Change
Wrap each instrumentation step (`prepare_to_enrich_errors`, `add_tagging`, `add_breadcrumbs`) in its own `try/except` and log a structured warning on failure (`sentry_prepare_failed`, `sentry_add_tagging_failed`, `sentry_add_breadcrumbs_failed`). The wrapped `run` still runs inside `sentry_sdk.new_scope` and its exceptions are captured via `sentry_sdk.capture_exception(e)` and re-raised — that path is unchanged.
## Test plan
- [x] `test_enrich_errors_isolates_instrumentation_from_task_run` (parametrised across all three instrumentation steps) — asserts (a) the wrapped `run()` still executes when each step raises, (b) the structured warning is emitted on the task log.
- [x] `test_enrich_errors_captures_and_reraises_task_failure` — asserts the wrapped-run failure path is unchanged: `sentry_sdk.capture_exception` is called, exception re-raised.
- [x] `prek run ruff` clean.
- [x] `prek run mypy-task-sdk` clean.
- [x] Full `test_sentry.py` suite: 10 passed.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Opus 4.7)
Generated-by: Claude Code (Opus 4.7) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Isolate Sentry instrumentation from task run [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #67505:
URL: https://github.com/apache/airflow/pull/67505#issuecomment-4627287228
Good call on the granularity — collapsed it. `prepare_to_enrich_errors` stays in its own guard (it runs `sentry_sdk.init()`), and `add_tagging` + `add_breadcrumbs` now share a single try/except (one `sentry_enrichment_failed` warning) instead of one each.
On "can these ever fail / all local operations" though — two of the three aren't local:
- **`add_breadcrumbs`** isn't local at all: it calls `RuntimeTaskInstance.get_task_breadcrumbs(...)`, which does `SUPERVISOR_COMMS.send(GetTaskBreadcrumbs(...))` — a blocking Execution-API/IPC round-trip to the supervisor. That can raise (IPC/API error), and today it would surface as a task failure.
- **`prepare_to_enrich_errors`** runs `sentry_sdk.init(dsn=…)` plus `conf.getimport("sentry", "before_send"/"transport")` — SDK/transport init and importing a configured callable can fail on misconfiguration.
- `add_tagging` is the one that's genuinely local (`scope.set_tag` + attribute reads), so you're right there — which is partly why it's now folded into the shared guard rather than its own.
So this isn't purely defensive against the impossible — `add_breadcrumbs` in particular is a remote call we don't want taking down a task. Mind another look?
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Isolate Sentry instrumentation from task run [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #67505:
URL: https://github.com/apache/airflow/pull/67505#discussion_r3374980413
##########
task-sdk/tests/task_sdk/execution_time/test_sentry.py:
##########
@@ -268,3 +268,75 @@ def test_minimum_config(self, mock_sentry_sdk, sentry_minimum):
sentry_minimum.prepare_to_enrich_errors(executor_integration="")
assert mock_sentry_sdk.integrations.logging.ignore_logger.mock_calls == [mock.call("airflow.task")]
assert mock_sentry_sdk.init.mock_calls == [mock.call(integrations=[])]
+
+ @pytest.mark.parametrize(
+ ("failing_step", "expected_event"),
+ [
+ ("prepare_to_enrich_errors", "sentry_prepare_failed"),
+ ("add_tagging", "sentry_enrichment_failed"),
+ ("add_breadcrumbs", "sentry_enrichment_failed"),
+ ],
+ )
+ def test_enrich_errors_isolates_instrumentation_from_task_run(
+ self,
+ mock_sentry_sdk,
+ sentry,
+ dag_run,
+ task_instance,
+ failing_step,
+ expected_event,
+ ):
+ """An instrumentation failure must not prevent the wrapped task from running.
+
+ ``add_tagging`` / ``add_breadcrumbs`` / ``prepare_to_enrich_errors`` exceptions used to
+ propagate as task failures (the wrapped ``run()`` never executed). ``prepare`` and the
+ ``add_tagging``/``add_breadcrumbs`` enrichment block are now guarded so instrumentation
+ is best-effort.
+ """
+ ran = {"value": False}
+
+ def fake_run(ti, context, log):
+ ran["value"] = True
+ return "result"
+
+ log = mock.MagicMock()
+ sentry_sdk_module = sys.modules["sentry_sdk"]
+ sentry_sdk_module.new_scope = mock.MagicMock()
+ sentry_sdk_module.new_scope.return_value.__enter__ = mock.MagicMock()
+ sentry_sdk_module.new_scope.return_value.__exit__ = mock.MagicMock(return_value=False)
Review Comment:
1. This leaks fixtures outside of this test.
2. This already exist as `mock_sentry_sdk` fixture and is used in the test right above this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Isolate Sentry instrumentation from task run [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #67505:
URL: https://github.com/apache/airflow/pull/67505#discussion_r3353480256
##########
task-sdk/src/airflow/sdk/execution_time/sentry/configured.py:
##########
@@ -135,15 +135,33 @@ def enrich_errors(self, run: Run) -> Run:
Wrap :func:`airflow.sdk.execution_time.task_runner.run` to support task
specific tags and breadcrumbs.
+
+ Sentry instrumentation is best-effort: ``prepare_to_enrich_errors``,
+ ``add_tagging`` and ``add_breadcrumbs`` are isolated from the wrapped
+ ``run(...)`` call so an instrumentation failure cannot block task
+ execution or surface as a task failure. The wrapped ``run`` still
+ runs inside ``sentry_sdk.new_scope`` and its exceptions are captured
+ and re-raised — that part is unchanged.
"""
@functools.wraps(run)
def wrapped_run(ti: RuntimeTaskInstance, context: Context, log: Logger) -> RunReturn:
- self.prepare_to_enrich_errors(ti.sentry_integration)
+ try:
+ self.prepare_to_enrich_errors(ti.sentry_integration)
+ except Exception:
+ log.warning("sentry_prepare_failed", exc_info=True)
+
with sentry_sdk.new_scope():
try:
self.add_tagging(context["dag_run"], ti)
+ except Exception:
+ log.warning("sentry_add_tagging_failed", exc_info=True)
+ try:
self.add_breadcrumbs(ti)
+ except Exception:
+ log.warning("sentry_add_breadcrumbs_failed", exc_info=True)
Review Comment:
A try/except for each is overkill.
Doubly so as these are all local operations. I don't think these can "ever" fail
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Isolate Sentry instrumentation from task run [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #67505:
URL: https://github.com/apache/airflow/pull/67505#issuecomment-4700364008
Fair question — I don't have a production occurrence. This is defensive: `add_tagging`/`add_breadcrumbs` call into the Sentry SDK, which can in principle raise (SDK bugs, odd tag-value serialization), and today such a raise is indistinguishable from a real task failure and can stop the wrapped `run()` from executing at all. The value is decoupling non-critical instrumentation from the critical run path, not fixing an observed bug. If the bar is a demonstrated failure, I'm happy to close it — no strong attachment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Isolate Sentry instrumentation from task run [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #67505:
URL: https://github.com/apache/airflow/pull/67505#issuecomment-4763500552
Closing then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Isolate Sentry instrumentation from task run [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #67505: Isolate Sentry instrumentation from task run
URL: https://github.com/apache/airflow/pull/67505
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org