From 977b306c4a66b5dd2b3944d1be63daf466cc4daf Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 27 Nov 2025 23:19:44 +0000 Subject: [PATCH] fix(spotlight): align behavior with SDK spec Align the Spotlight implementation with the official spec at https://develop.sentry.dev/sdk/expected-features/spotlight/ Changes: 1. Fix precedence rules: - When spotlight=True and SENTRY_SPOTLIGHT env var is a URL, use the env var URL (per spec requirement) - Log warning when config URL overrides env var URL - Log warning when spotlight=False explicitly disables despite env var being set 2. Route all envelopes to Spotlight: - Sessions, logs, and metrics now also get sent to Spotlight via the _capture_envelope callback - Move Spotlight initialization before batchers are created 3. Add exponential backoff retry logic: - SpotlightClient now implements proper exponential backoff when the Spotlight server is unreachable - Skips sending during backoff period to avoid blocking - Logs errors only once per backoff cycle 4. Update tests: - Fix test expectation for spotlight=True + env URL case - Add tests for warning scenarios - Add test for session envelope routing to Spotlight --- sentry_sdk/client.py | 88 ++++++++++++++++++++++++++++++----------- sentry_sdk/spotlight.py | 55 +++++++++++++++++++++----- tests/test_client.py | 3 +- tests/test_spotlight.py | 67 +++++++++++++++++++++++++++++++ 4 files changed, 179 insertions(+), 34 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index c831675314..ed290b8f5c 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -360,6 +360,8 @@ def _init_impl(self): def _capture_envelope(envelope): # type: (Envelope) -> None + if self.spotlight is not None: + self.spotlight.capture_envelope(envelope) if self.transport is not None: self.transport.capture_envelope(envelope) @@ -387,6 +389,69 @@ def _record_lost_event( if self.options["enable_backpressure_handling"]: self.monitor = Monitor(self.transport) + # Setup Spotlight before creating batchers so _capture_envelope can use it + spotlight_config = self.options.get("spotlight") + spotlight_env_value = os.environ.get("SENTRY_SPOTLIGHT") + + # Parse env var to determine if it's a boolean or URL + spotlight_from_env = None # type: Optional[bool] + spotlight_env_url = None # type: Optional[str] + if spotlight_env_value: + parsed = env_to_bool(spotlight_env_value, strict=True) + if parsed is None: + # It's a URL string + spotlight_from_env = True + spotlight_env_url = spotlight_env_value + else: + spotlight_from_env = parsed + + # Apply precedence rules per spec + if spotlight_config is False: + # Config explicitly disables spotlight - warn if env var was set + if spotlight_from_env: + logger.warning( + "Spotlight is disabled via spotlight=False config option, " + "ignoring SENTRY_SPOTLIGHT environment variable." + ) + self.options["spotlight"] = False + elif spotlight_config is True: + # Config enables spotlight with boolean true + # If env var has URL, use env var URL per spec + if spotlight_env_url: + self.options["spotlight"] = spotlight_env_url + else: + self.options["spotlight"] = True + elif isinstance(spotlight_config, str): + # Config has URL string - use config URL, warn if env var differs + if spotlight_env_value and spotlight_env_value != spotlight_config: + logger.warning( + "Spotlight URL from config (%s) takes precedence over " + "SENTRY_SPOTLIGHT environment variable (%s).", + spotlight_config, + spotlight_env_value, + ) + # self.options["spotlight"] already has the config URL + elif spotlight_config is None: + # No config - use env var + if spotlight_env_url: + self.options["spotlight"] = spotlight_env_url + elif spotlight_from_env: + self.options["spotlight"] = True + # else: stays None (disabled) + + if self.options.get("spotlight"): + # This is intentionally here to prevent setting up spotlight + # stuff we don't need unless spotlight is explicitly enabled + from sentry_sdk.spotlight import setup_spotlight + + self.spotlight = setup_spotlight(self.options) + if not self.options["dsn"]: + sample_all = lambda *_args, **_kwargs: 1.0 + self.options["send_default_pii"] = True + self.options["error_sampler"] = sample_all + self.options["traces_sampler"] = sample_all + self.options["profiles_sampler"] = sample_all + self.session_flusher = SessionFlusher(capture_func=_capture_envelope) self.log_batcher = None @@ -437,29 +502,6 @@ def _record_lost_event( options=self.options, ) - spotlight_config = self.options.get("spotlight") - if spotlight_config is None and "SENTRY_SPOTLIGHT" in os.environ: - spotlight_env_value = os.environ["SENTRY_SPOTLIGHT"] - spotlight_config = env_to_bool(spotlight_env_value, strict=True) - self.options["spotlight"] = ( - spotlight_config - if spotlight_config is not None - else spotlight_env_value - ) - - if self.options.get("spotlight"): - # This is intentionally here to prevent setting up spotlight - # stuff we don't need unless spotlight is explicitly enabled - from sentry_sdk.spotlight import setup_spotlight - - self.spotlight = setup_spotlight(self.options) - if not self.options["dsn"]: - sample_all = lambda *_args, **_kwargs: 1.0 - self.options["send_default_pii"] = True - self.options["error_sampler"] = sample_all - self.options["traces_sampler"] = sample_all - self.options["profiles_sampler"] = sample_all - sdk_name = get_sdk_name(list(self.integrations.keys())) SDK_INFO["name"] = sdk_name logger.debug("Setting SDK name to '%s'", sdk_name) diff --git a/sentry_sdk/spotlight.py b/sentry_sdk/spotlight.py index 4ac427b9c1..8e7236b98b 100644 --- a/sentry_sdk/spotlight.py +++ b/sentry_sdk/spotlight.py @@ -1,6 +1,7 @@ import io import logging import os +import time import urllib.parse import urllib.request import urllib.error @@ -34,14 +35,39 @@ class SpotlightClient: + """ + A client for sending envelopes to Sentry Spotlight. + + Implements exponential backoff retry logic per the SDK spec: + - Logs error at least once when server is unreachable + - Does not log for every failed envelope + - Uses exponential backoff to avoid hammering an unavailable server + - Never blocks normal Sentry operation + """ + + # Exponential backoff settings + INITIAL_RETRY_DELAY = 1.0 # Start with 1 second + MAX_RETRY_DELAY = 60.0 # Max 60 seconds + def __init__(self, url): # type: (str) -> None self.url = url self.http = urllib3.PoolManager() - self.fails = 0 + self._error_logged = False + self._retry_delay = self.INITIAL_RETRY_DELAY + self._last_error_time = 0.0 # type: float def capture_envelope(self, envelope): # type: (Envelope) -> None + + # Check if we're in backoff period - skip sending to avoid blocking + current_time = time.time() + if self._last_error_time > 0: + time_since_error = current_time - self._last_error_time + if time_since_error < self._retry_delay: + # Still in backoff period, skip this envelope + return + body = io.BytesIO() envelope.serialize_into(body) try: @@ -54,18 +80,27 @@ def capture_envelope(self, envelope): }, ) req.close() - self.fails = 0 + # Success - reset backoff state + self._error_logged = False + self._retry_delay = self.INITIAL_RETRY_DELAY + self._last_error_time = 0.0 except Exception as e: - if self.fails < 2: - sentry_logger.warning(str(e)) - self.fails += 1 - elif self.fails == 2: - self.fails += 1 + current_time = time.time() + self._last_error_time = current_time + + # Log error only once per backoff cycle + if not self._error_logged: sentry_logger.warning( - "Looks like Spotlight is not running, will keep trying to send events but will not log errors." + "Failed to send envelope to Spotlight at %s: %s. " + "Will retry after %.1f seconds.", + self.url, + e, + self._retry_delay, ) - # omitting self.fails += 1 in the `else:` case intentionally - # to avoid overflowing the variable if Spotlight never becomes reachable + self._error_logged = True + + # Increase backoff delay exponentially + self._retry_delay = min(self._retry_delay * 2, self.MAX_RETRY_DELAY) try: diff --git a/tests/test_client.py b/tests/test_client.py index a5b0b44931..35edfdb5b7 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1181,7 +1181,8 @@ def test_debug_option( (None, "t", DEFAULT_SPOTLIGHT_URL), (None, "1", DEFAULT_SPOTLIGHT_URL), (True, None, DEFAULT_SPOTLIGHT_URL), - (True, "http://localhost:8080/slurp", DEFAULT_SPOTLIGHT_URL), + # Per spec: spotlight=True + env URL -> use env URL + (True, "http://localhost:8080/slurp", "http://localhost:8080/slurp"), ("http://localhost:8080/slurp", "f", "http://localhost:8080/slurp"), (None, "http://localhost:8080/slurp", "http://localhost:8080/slurp"), ], diff --git a/tests/test_spotlight.py b/tests/test_spotlight.py index d00c4eb8fc..f554ff7c5b 100644 --- a/tests/test_spotlight.py +++ b/tests/test_spotlight.py @@ -1,6 +1,7 @@ import pytest import sentry_sdk +from sentry_sdk.spotlight import DEFAULT_SPOTLIGHT_URL @pytest.fixture @@ -54,3 +55,69 @@ def test_spotlight_envelope(sentry_init, capture_spotlight_envelopes): payload = envelope.items[0].payload.json assert payload["exception"]["values"][0]["value"] == "aha!" + + +def test_spotlight_true_with_env_url_uses_env_url(sentry_init, monkeypatch): + """Per spec: spotlight=True + env URL -> use env URL""" + monkeypatch.setenv("SENTRY_SPOTLIGHT", "http://custom:9999/stream") + sentry_init(spotlight=True) + + spotlight = sentry_sdk.get_client().spotlight + assert spotlight is not None + assert spotlight.url == "http://custom:9999/stream" + + +def test_spotlight_false_ignores_env_var(sentry_init, monkeypatch, caplog): + """Per spec: spotlight=False ignores env var and logs warning""" + import logging + + with caplog.at_level(logging.WARNING, logger="sentry_sdk.errors"): + monkeypatch.setenv("SENTRY_SPOTLIGHT", "true") + sentry_init(spotlight=False, debug=True) + + assert sentry_sdk.get_client().spotlight is None + assert "ignoring SENTRY_SPOTLIGHT environment variable" in caplog.text + + +def test_spotlight_config_url_overrides_env_url_with_warning( + sentry_init, monkeypatch, caplog +): + """Per spec: config URL takes precedence over env URL with warning""" + import logging + + with caplog.at_level(logging.WARNING, logger="sentry_sdk.errors"): + monkeypatch.setenv("SENTRY_SPOTLIGHT", "http://env:9999/stream") + sentry_init(spotlight="http://config:8888/stream", debug=True) + + spotlight = sentry_sdk.get_client().spotlight + assert spotlight is not None + assert spotlight.url == "http://config:8888/stream" + assert "takes precedence over" in caplog.text + + +def test_spotlight_config_url_same_as_env_no_warning(sentry_init, monkeypatch, caplog): + """No warning when config URL matches env URL""" + import logging + + with caplog.at_level(logging.WARNING, logger="sentry_sdk.errors"): + monkeypatch.setenv("SENTRY_SPOTLIGHT", "http://same:9999/stream") + sentry_init(spotlight="http://same:9999/stream", debug=True) + + spotlight = sentry_sdk.get_client().spotlight + assert spotlight is not None + assert spotlight.url == "http://same:9999/stream" + assert "takes precedence over" not in caplog.text + + +def test_spotlight_receives_session_envelopes(sentry_init, capture_spotlight_envelopes): + """Spotlight should receive session envelopes, not just error events""" + sentry_init(spotlight=True, release="test-release") + envelopes = capture_spotlight_envelopes() + + # Start and end a session + sentry_sdk.get_isolation_scope().start_session() + sentry_sdk.get_isolation_scope().end_session() + sentry_sdk.flush() + + # Should have received at least one envelope with session data + assert len(envelopes) > 0