Dashboard › getsentry › Session 02Wr38QhF3rw
02Wr38QhF3rwyuwQPYou are reviewing PR #21336 in getsentry/getsentry ("fix(orbital): restore notify metrics, IP fallback, and Service DNS"). The branch is fix/orbital-volume-metrics-ip-dns. The diff is the last 6 commits on that branch (everything since the merge-base with master).
Your job is to do an INDEPENDENT code review from scratch — as if you had no prior context. Be thorough, skeptical, and adversarial. Find problems. Do not assume the existing reviews are correct or complete.
The three modified files are:
getsentry/conf/settings/cellsilo.py (US cell default DNS)getsentry/receivers/orbital.py (notify_orbital publisher + metrics)tests/getsentry/receivers/test_orbital.py (tests)The PR purpose: restore notify_orbital to emit per-event metrics (getsentry.orbital.notify with outcome tags), fall back to event.data.user.ip_address when Relay's remote_addr is missing, switch the US default from a stale Internal LB IP to Service DNS, and avoid spamming no_socket metric in non-US cells where ORBITAL_UDP_SERVER is intentionally unset.
Please perform a thorough review and report:
Correctness: Does the code do what it claims? Specifically:
Code quality:
Test quality:
Planning parallel GitHub status and diff checks
diff --git a/getsentry/conf/settings/cellsilo.py b/getsentry/conf/settings/cellsilo.py
index c8f5fcffc5b9..354e6f7359d0 100644
--- a/getsentry/conf/settings/cellsilo.py
+++ b/getsentry/conf/settings/cellsilo.py
@@ -2293,8 +2293,12 @@ def update_kafka_slices(topic_basename: str, cluster: str, envvar: str):
}
# Orbital — only runs in the US cell; elsewhere leave unset so no socket is opened.
+# Prefer the in-cluster Service DNS over a stale Internal LB IP (10.2.0.43 was
+# hardcoded since 2019 and no longer reaches all orbital pods reliably). The
+# trailing dot makes the name FQDN so pod-side `ndots:5` doesn't walk search
+# domains on every sendto (same pattern as twemproxy-…svc.cluster.local.).
if SENTRY_LOCAL_CELL == "us":
- ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER", "10.2.0.43:5556")
+ ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER", "orbital-udp.default.svc.cluster.local.:5556")
else:
ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER") or None
diff --git ...
author: BYK association: member edited: false status: changes requested -- -- author: cursor association: none edited: false status: commented -- <!-- BUGBOT_REVIEW --> Cursor Bugbot has reviewed your changes and found 1 potential issue. <!-- BUGBOT_FIX_ALL --> <a href="https://cursor.com/open?link=eyJ2ZXJzaW9uIjoxLCJ0eXBlIjoiQlVHQk9UX0ZJWF9BTExfSU5fQ1VSU09SIiwiZGF0YSI6eyJyZWRpc0tleSI6ImJ1Z2JvdC1tdWx0aTo0OGIwMmQyMS0xNzBiLTQyYzQtYWUyYS1kYWUzNTg5YmU0MDEiLCJlbmNyeXB0aW9uS2V5IjoieFJPTUpXVUkzQUxSN1hRREhTM2ZIeTFsM0ZaUG02aXhLWEctY3J2RDZ1VSIsImJyYW5jaCI6ImZpeC9vcmJpdGFsLXZvbHVtZS1tZXRyaWNzLWlwLWRucyIsInJlcG9Pd25lciI6ImdldHNlbnRyeSIsInJlcG9OYW1lIjoiZ2V0c2VudHJ5IiwicHJvdmlkZXIiOiJnaXRodWIifX0" target="_blank" rel="noopener noreferrer"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/fix-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/fix-in-cursor-light.png"><img alt="Fix All in Cursor" ...
[{"url":"https://api.github.com/repos/getsentry/getsentry/pulls/comments/3697954507","pull_request_review_id":4837320505,"id":3697954507,"node_id":"PRRC_kwDOAC60vc7cak7L","diff_hunk":"@@ -2293,8 +2293,10 @@ def update_kafka_slices(topic_basename: str, cluster: str, envvar: str):\n }\n \n # Orbital — only runs in the US cell; elsewhere leave unset so no socket is opened.\n+# Prefer the in-cluster Service DNS over a stale Internal LB IP (10.2.0.43 was\n+# hardcoded since 2019 and no longer reaches all orbital pods reliably).\n if SENTRY_LOCAL_CELL == \"us\":\n- ORBITAL_UDP_SERVER = env(\"ORBITAL_UDP_SERVER\", \"10.2.0.43:5556\")\n+ ORBITAL_UDP_SERVER = env(\"ORBITAL_UDP_SERVER\", ...
Let me read the actual files to do an independent review, not just rely on the diff. I also need to look at the surrounding code in the cellsilo settings, the getsentry settings module, and the signal infrastructure. Let me also check the test setup and dev settings to understand the no_socket/no-op interaction with the real test setup.
<path>/home/byk/Code/getsentry/getsentry/getsentry/receivers/orbital.py</path>
<type>file</type>
<content>
1: import socket
2: from time import time
3: from typing import Any
4:
5: from sentry.signals import event_accepted
6: from sentry.utils import metrics
7: from sentry.utils.geo import geo_by_addr
8: from sentry.utils.json import dumps
9:
10: from getsentry.settings import ORBITAL_UDP_SERVER
11:
12: # This powers live.sentry.io.
13:
14: udp_socket: socket.socket | None = None
15: udp_addr: tuple[str, int] | None = None
16:
17: if ORBITAL_UDP_SERVER:
18: try:
19: host, port = ORBITAL_UDP_SERVER.rsplit(":", 1)
20: udp_addr = (host, int(port))
21: udp_socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
22: except Exception:
23: # ORBITAL_UDP_SERVER is set but the socket failed to initialize (likely
24: # malformed value). ...
<path>/home/byk/Code/getsentry/getsentry/tests/getsentry/receivers/test_orbital.py</path> <type>file</type> <content> 1: import importlib 2: from unittest import mock 3: from unittest.mock import MagicMock, patch 4: 5: from sentry.utils import json 6: 7: from getsentry import settings as getsentry_settings 8: from getsentry.receivers import orbital 9: from getsentry.testutils import GetsentryTestCase 10: 11: 12: class NotifyOrbitalTest(GetsentryTestCase): 13: def _outcomes(self, incr: MagicMock) -> list[str]: 14: return [call.kwargs["tags"]["outcome"] for call in incr.call_args_list] 15: 16: def test_notify_orbital_is_noop_when_udp_socket_unset(self) -> None: 17: """In non-US cells ORBITAL_UDP_SERVER is unset, so notify_orbital is 18: bound to a noop at module load and emits no per-event metrics. 19: 20: dev.py sets ORBITAL_UDP_SERVER to a valid value, so udp_socket is 21: always initialized in tests. ...
<path>/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/cellsilo.py</path>
<type>file</type>
<content>
2280: # Jira
2281: JIRA_USE_EMAIL_SCOPE = True
2282:
2283: ################
2284: # Chartcuterie #
2285: ################
2286: SENTRY_OPTIONS["chart-rendering.enabled"] = True
2287: SENTRY_OPTIONS["chart-rendering.chartcuterie"] = {"url": "http://chartcuterie"}
2288: SENTRY_OPTIONS["chart-rendering.storage.backend"] = (
2289: "sentry.services.filestore.gcs.GoogleCloudStorage"
2290: )
2291: SENTRY_OPTIONS["chart-rendering.storage.options"] = {
2292: "bucket_name": env("CHARTCUTERIE_BUCKET", "sentryio-chartcuterie-bucket")
2293: }
2294:
2295: # Orbital — only runs in the US cell; elsewhere leave unset so no socket is opened.
2296: # Prefer the in-cluster Service DNS over a stale Internal LB IP (10.2.0.43 was
2297: # hardcoded since 2019 and no longer reaches all orbital pods reliably). ...
Now let me check the getsentry.settings module to understand the type annotation for ORBITAL_UDP_SERVER, and check how other settings handle the env pattern. Let me also check BetterSignal to understand how it handles dispatch_uid with weak=False, and check if there are any quirks with the test setup.
Found 21 matches /home/byk/Code/getsentry/getsentry/tests/getsentry/receivers/test_orbital.py: Line 17: """In non-US cells ORBITAL_UDP_SERVER is unset, so notify_orbital is Line 20: dev.py sets ORBITAL_UDP_SERVER to a valid value, so udp_socket is Line 22: orbital with ORBITAL_UDP_SERVER empty, mirroring the cellsilo.py branch Line 24: original_server = getsentry_settings.ORBITAL_UDP_SERVER Line 25: getsentry_settings.ORBITAL_UDP_SERVER = "" Line 36: getsentry_settings.ORBITAL_UDP_SERVER = original_server Line 40: """When ORBITAL_UDP_SERVER is set but malformed, the module emits the Line 44: original_server = getsentry_settings.ORBITAL_UDP_SERVER Line 45: getsentry_settings.ORBITAL_UDP_SERVER = "not-a-valid-endpoint" Line 55: getsentry_settings.ORBITAL_UDP_SERVER = original_server /home/byk/Code/getsentry/getsentry/getsentry/conf/settings/cellsilo.py: Line 2301: ...
No files found
/home/byk/Code/getsentry/getsentry/getsentry/settings.py
<path>/home/byk/Code/getsentry/getsentry/getsentry/settings.py</path>
<type>file</type>
<content>
1: import os
2: import sys
3:
4: import stripe
5:
6: ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
7:
8: if ROOT not in sys.path:
9: sys.path.insert(0, ROOT)
10:
11: # Import our defaults (globals)
12:
13: from getsentry.conf.settings.defaults import * # NOQA
14:
15: # Inherit from environment specifics
16:
17: if "GETSENTRY_DJANGO_CONF" in os.environ:
18: DJANGO_CONF = os.environ["GETSENTRY_DJANGO_CONF"]
19: else:
20: DJANGO_CONF = os.environ.get("DJANGO_CONF", "dev")
21: if DJANGO_CONF != "defaults":
22: config = "getsentry.conf.settings.%s" % DJANGO_CONF
23: sys.stderr.write("Using configuration %r\n" % config)
24: module = __import__(config, globals(), locals(), ["*"])
25: for k in dir(module):
26: locals()[k] = getattr(module, k)
27:
28: stripe.api_key = locals()["STRIPE_API_KEY"]
29: stripe.api_version = ...
<path>/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/defaults.py</path>
<type>file</type>
<content>
1050:
1051: # Intercom
1052: SENTRY_INTERCOM_APP_ID = env("INTERCOM_APP_ID")
1053: SENTRY_INTERCOM_API_SECRET = env("INTERCOM_API_SECRET")
1054:
1055: ALLOWED_CREDENTIAL_ORIGINS = []
1056:
1057: ORBITAL_UDP_SERVER = ""
1058:
1059: BITBUCKET_CONSUMER_KEY = "7q3JA92cNdgfb2WneG"
(Showing lines 1050-1059 of 1537. Use offset=1060 to continue.)
</content>
<path>/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/dev.py</path>
<type>file</type>
<content>
140: "BACKEND": "sentry.cache.backends.reconnectingmemcache.ReconnectingMemcache",
141: "LOCATION": "127.0.0.1:11211",
142: "OPTIONS": {"ignore_exc": True, "reconnect_age": 300},
143: }
144: }
145:
146: ORBITAL_UDP_SERVER = "127.0.0.1:5556"
147:
148: if os.environ.get("SENTRY_DEVSERVICES_DSN"):
149: parts = urlparse(os.environ["SENTRY_DEVSERVICES_DSN"])
(Showing lines 140-149 of 314. Use offset=150 to continue.)
</content>
<path>/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/singletenant.py</path> <type>file</type> <content> 1080: SENTRY_ORGANIZATION_BASE_HOSTNAME = "" 1081: SENTRY_ORGANIZATION_URL_TEMPLATE = "" 1082: SENTRY_REGION_API_URL_TEMPLATE = "" 1083: SENTRY_OPTIONS["system.region"] = "" 1084: 1085: ORBITAL_UDP_SERVER = "" 1086: 1087: # TODO(mark) Figure out what this needs to be for single tenants that have slack setup. 1088: SLACK_CUSTOMERS_WEBHOOK = "" 1089: (Showing lines 1080-1089 of 1166. Use offset=1090 to continue.) </content>
<path>/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/controlsilo.py</path>
<type>file</type>
<content>
830: SENTRY_FEATURES["relocation:enabled"] = True
831:
832: ###########
833: # Orbital #
834: ###########
835: ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER") or None
836:
837: ##############
838: # Newsletter #
839: ##############
(Showing lines 830-839 of 990. Use offset=840 to continue.)
</content>
Now I need to check a few critical things:
Where is getsentry.settings.ORBITAL_UDP_SERVER typed? The test imports from getsentry import settings as getsentry_settings and accesses getsentry_settings.ORBITAL_UDP_SERVER. But looking at the settings.py file, this module just imports from getsentry.conf.settings.defaults and conditionally from a DJANGO_CONF specific module. So getsentry_settings.ORBITAL_UDP_SERVER is essentially a global attribute set by settings files. There's no type annotation in the settings.py module. The default is "" (empty string).
So getsentry_settings.ORBITAL_UDP_SERVER could be:
"" (defaults.py)"127.0.0.1:5556" (dev.py)"" (singletenant.py)env("ORBITAL_UDP_SERVER") or None (controlsilo.py) — could be None!"orbital-udp.default.svc.cluster.local.:5556" or None (cellsilo.py) — could be None!"10.2.0.43:5556" previously (the old value)So the type is actually str | None. ...
Found 64 matches /home/byk/Code/getsentry/sentry/src/sentry/models/group.py: Line 1244: @receiver(pre_save, sender=Group, dispatch_uid="pre_save_group_default_substatus", weak=False) /home/byk/Code/getsentry/sentry/src/sentry/models/projectcodeowners.py: Line 167: dispatch_uid="projectcodeowners_modify_date_updated", /home/byk/Code/getsentry/sentry/src/sentry/models/releases/util.py: Line 306: parse_semver_pre_save, sender="sentry.Release", dispatch_uid="parse_semver_pre_save" /home/byk/Code/getsentry/sentry/src/sentry/workflow_engine/receivers/project_workflows.py: Line 95: dispatch_uid="create_default_workflows", /home/byk/Code/getsentry/sentry/src/sentry/workflow_engine/receivers/project_detectors.py: Line 29: create_project_detectors, sender=Project, dispatch_uid="create_project_detectors" Line 32: create_default_anomaly_detector, dispatch_uid="create_default_anomaly_detector" Line 41: ...
No files found
Found 1 matches /home/byk/Code/getsentry/sentry/src/sentry/signals.py: Line 57: class BetterSignal(Signal):
No files found
<path>/home/byk/Code/getsentry/sentry/src/sentry/signals.py</path>
<type>file</type>
<content>
1: from __future__ import annotations
2:
3: import enum
4: import functools
5: import logging
6: from collections.abc import Callable
7: from typing import Any
8:
9: from django.dispatch.dispatcher import Signal
10:
11: from sentry.utils.env import in_test_environment
12:
13: Receiver = Callable[[], Any]
14:
15: _AllReceivers = enum.Enum("_AllReceivers", "ALL")
16:
17:
18: _receivers_that_raise: _AllReceivers | list[Receiver] = []
19:
20:
21: class receivers_raise_on_send:
22: """
23: Testing utility that forces send_robust to raise, rather than return, exceptions for signal receivers
24: that match the given receivers within the context. The default receivers mode is to raise all receiver exceptions.
25:
26: This behavior only works in tests. ...
Found 5 matches /home/byk/Code/getsentry/sentry/src/sentry/signals.py: Line 99: event_accepted = BetterSignal() # ["ip", "data", "project"] /home/byk/Code/getsentry/sentry/src/sentry/ingest/consumer/processors.py: Line 29: from sentry.signals import event_accepted Line 312: # emit event_accepted once everything is done Line 313: with start_span(op="event_accepted.send_robust", name="event_accepted.send_robust"): Line 314: event_accepted.send_robust(
Now let me look at the actual call site of event_accepted in ingest/consumer/processors.py to understand what gets passed.
<path>/home/byk/Code/getsentry/sentry/src/sentry/ingest/consumer/processors.py</path>
<type>file</type>
<content>
295: ):
296: preprocess_kwargs: dict[str, Any] = {
297: "cache_key": cache_key or "",
298: "data": data,
299: "start_time": start_time,
300: "event_id": event_id,
301: "project": project,
302: "has_attachments": bool(attachments),
303: }
304: if inline_save_event:
305: preprocess_kwargs["inline_save_event"] = True
306: preprocess_event(**preprocess_kwargs)
307:
308: # remember for an 1 hour that we saved this event (deduplication protection)
309: with start_span(op="cache.set", name="cache.set"):
310: cache.set(deduplication_key, "", CACHE_TIMEOUT)
311:
312: # emit event_accepted once everything is done
313: with ...
<path>/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/cellsilo.py</path>
<type>file</type>
<content>
2295: # Orbital — only runs in the US cell; elsewhere leave unset so no socket is opened.
2296: # Prefer the in-cluster Service DNS over a stale Internal LB IP (10.2.0.43 was
2297: # hardcoded since 2019 and no longer reaches all orbital pods reliably). The
2298: # trailing dot makes the name FQDN so pod-side `ndots:5` doesn't walk search
2299: # domains on every sendto (same pattern as twemproxy-…svc.cluster.local.).
2300: if SENTRY_LOCAL_CELL == "us":
2301: ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER", "orbital-udp.default.svc.cluster.local.:5556")
2302: else:
2303: ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER") or None
2304:
2305: #################
2306: # Feature flags #
2307: #################
2308: SENTRY_FEATURES["organizations:monitors"] = True
2309:
2310: # Organization ingest subdomains only setup in saas
2311: ...
Now let me check how data looks like. Is it the raw dict with user key, or is it a wrapped object? Let me look at how it's processed.
Found 2 matches
/home/byk/Code/getsentry/sentry/src/sentry/ingest/consumer/processors.py:
Line 95: remote_addr = message.get("remote_addr")
Line 315: ip=remote_addr, data=data, project=project, sender=process_event
No files found
Found 11 matches
/home/byk/Code/getsentry/sentry/src/sentry/ingest/consumer/processors.py:
Line 18: from sentry.feedback.usecases.ingest.userreport import Conflict, save_userreport
Line 405: @trace_func(name="ingest_consumer.process_userreport")
Line 406: @metrics.wraps("ingest_consumer.process_userreport")
Line 407: def process_userreport(message: IngestMessage, project: Project) -> bool:
Line 412: save_userreport(
Line 420: logger.info("Invalid userreport: %s", e)
Line 425: logger.exception("userreport.save.crash")
/home/byk/Code/getsentry/sentry/src/sentry/ingest/consumer/factory.py:
Line 145: # - `user_report`: user reports, which are also emitted on this topic.
/home/byk/Code/getsentry/sentry/src/sentry/ingest/consumer/attachment_event.py:
Line 20: process_userreport,
Line 108: elif message_type == "user_report":
Line 109: process_userreport(message, project)
<path>/home/byk/Code/getsentry/sentry/src/sentry/ingest/consumer/processors.py</path>
<type>file</type>
<content>
80: consumer_type: str,
81: message: IngestMessage,
82: project: Project,
83: reprocess_only_stuck_events: bool = False,
84: reprocess_only_events_not_in_nodestore: bool = False,
85: inline_save_event: bool = False,
86: inline_save_event_transaction: bool = False,
87: ) -> None:
88: """
89: Perform some initial filtering and deserialize the message payload.
90: """
91: payload = message["payload"]
92: start_time = float(message["start_time"])
93: event_id = message["event_id"]
94: project_id = int(message["project_id"])
95: remote_addr = message.get("remote_addr")
96: attachments = message.get("attachments") or ()
97:
98: sentry_sdk.set_extra("event_id", event_id)
99: sentry_sdk.set_attribute("event_id", event_id)
100: sentry_sdk.set_extra("len_attachments", len(attachments))
101: ...
Important: data = orjson.loads(payload). So data is the raw event payload (a dict), and user would be data.get("user") which is the user dict in the event payload. Good, this is what the new code expects.
But wait — the user.ip_address field on events is actually set by client SDKs through context. For events, the user dict typically has keys like id, email, username, ip_address. Let me check the IP-fallback behavior in production for typical event payloads.
But the key question is: when the IP comes from data.get("user").get("ip_address") on the event payload, is it potentially a private/internal IP that should not be geo-located? For example, if a user sets {{auto}} as the IP, Relay might fill in the actual remote_addr. If the SDK explicitly sets it, it could be anything. The geo_by_addr function will fail to lookup some private IPs (10.x, 192.168.x, etc.) and return None or raise.
The bigger question is: what if user.ip_address is set to a ...
Found 1 matches /home/byk/Code/getsentry/sentry/src/sentry/utils/geo.py: Line 13: def geo_by_addr(ip: str) -> dict[str, Any] | None:
Found 32 matches /home/byk/Code/getsentry/sentry/src/sentry/utils/samples.py: Line 92: def generate_user(username=None, email=None, ip_address=None, id=None): Line 101: "ip_address": ip_address or random_ip(), Line 277: generate_user(ip_address="127.0.0.1", username="sentry", id=1, email="sentry@example.com"), /home/byk/Code/getsentry/sentry/src/sentry/utils/mockdata/core.py: Line 1293: "ip_address": "72.164.175.154", /home/byk/Code/getsentry/sentry/src/sentry/utils/eventuser.py: Line 9: from ipaddress import IPv4Address, IPv6Address, ip_address Line 60: "ip_address": "ip", Line 70: def get_ip_address_conditions(ip_addresses: Sequence[str]) -> list[Condition]: Line 73: Silently ignores invalid IP addresses, and applies `Op.IN` to the `ip_address_v4` and/or `ip_address_v6` columns. Line 77: for ip in ip_addresses: Line 79: valid_ip = ip_address(ip) Line 89: ...
<path>/home/byk/Code/getsentry/sentry/src/sentry/utils/geo.py</path>
<type>file</type>
<content>
1: from __future__ import annotations
2:
3: import logging
4: from typing import Any
5:
6: from django.conf import settings
7: from sentry_relay.processing import GeoIpLookup
8:
9: logger = logging.getLogger(__name__)
10:
11:
12: # default is no-op
13: def geo_by_addr(ip: str) -> dict[str, Any] | None:
14: return None
15:
16:
17: rust_geoip: None | GeoIpLookup = None
18:
19:
20: def _init_geoip() -> None:
21: global geo_by_addr
22:
23: import maxminddb
24:
25: assert settings.GEOIP_PATH_MMDB is not None # checked below
26:
27: try:
28: geo_db = maxminddb.open_database(settings.GEOIP_PATH_MMDB, maxminddb.MODE_AUTO)
29: except Exception:
30: logger.warning("Error opening GeoIP database: %s", settings.GEOIP_PATH_MMDB)
31: return
32:
33: def _geo_by_addr(ip: str) -> dict[str, Any] | None:
34: rv = geo_db.get(ip)
35: ...
Let me check what happens in the test environment for geo_by_addr (default returns None). So in tests, geo_by_addr returns None, which means it doesn't raise. But the tests mock geo_by_addr, so that's fine.
Now let me check the production geo_by_addr Rust implementation. The Rust code can return None for unknown IPs (e.g., private IPs like 10.x, 192.168.x, etc.) but it won't raise an exception for those. The Python _geo_by_addr returns None for unknown IPs too. So geo_error would only fire for truly malformed input that the maxminddb library can't even parse.
Wait — let me check the previous version of notify_orbital to see if it worked the same way:
Actually, the previous version didn't have metrics. So the behavior was just to silently return. The new code adds metrics, so geo_error is new behavior. The exception path catches all exceptions, which is fine.
Now let me check how notify_orbital is bound to event_accepted and how this gets called. ...
Found 8 matches
/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/cellsilo.py:
Line 1133: _twemproxy_pool = env("TWEMPROXY_POOL", default="cache")
Line 1135: env("CACHE_DEFAULT_HOST") or f"twemproxy-{_twemproxy_pool}.default.svc.cluster.local.:11211"
Line 1138: env("CACHE_NODEDATA_HOST") or "twemproxy-nodedata.default.svc.cluster.local.:11211"
Line 1143: _twemproxy_options_pool = env("TWEMPROXY_OPTIONS_POOL", default=_twemproxy_pool)
Line 1146: or f"twemproxy-{_twemproxy_options_pool}.default.svc.cluster.local.:11211"
Line 2299: # domains on every sendto (same pattern as twemproxy-…svc.cluster.local.).
/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/controlsilo.py:
Line 249: twemproxy_pool = env("TWEMPROXY_POOL", default="cache")
Line 254: "LOCATION": [f"twemproxy-{twemproxy_pool}.default.svc.cluster.local.:11211"],
Found 8 matches
/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/controlsilo.py:
Line 254: "LOCATION": [f"twemproxy-{twemproxy_pool}.default.svc.cluster.local.:11211"],
/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/cellsilo.py:
Line 1135: env("CACHE_DEFAULT_HOST") or f"twemproxy-{_twemproxy_pool}.default.svc.cluster.local.:11211"
Line 1138: env("CACHE_NODEDATA_HOST") or "twemproxy-nodedata.default.svc.cluster.local.:11211"
Line 1146: or f"twemproxy-{_twemproxy_options_pool}.default.svc.cluster.local.:11211"
Line 2299: # domains on every sendto (same pattern as twemproxy-…svc.cluster.local.).
Line 2301: ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER", "orbital-udp.default.svc.cluster.local.:5556")
/home/byk/Code/getsentry/getsentry/getsentry/conf/settings/singletenant.py:
Line 393: "KAFKA_BOOTSTRAP_SERVERS", default="ingest-kafka.default.svc.cluster.local:9092"
Line 505: ...
Good — the twemproxy pattern does use the trailing dot (cluster.local.:11211). The new orbital default is consistent with that pattern. The cursor bugbot review was correct that the original PR (commit 6822013) had a missing trailing dot, and the fix added it.
But notice: KAFKA_BOOTSTRAP_SERVERS in singletenant.py does NOT use the trailing dot (cluster.local:9092). So the pattern is mixed in the codebase. The new orbital default is consistent with the twemproxy pattern (with trailing dot), which is the correct one for hot-path DNS.
Now let me check the test environment more carefully. The test uses GetsentryTestCase. Let me check what settings are loaded for tests and what ORBITAL_UDP_SERVER is at test time.
Found 1 matches /home/byk/Code/getsentry/getsentry/getsentry/testutils/cases.py: Line 124: class GetsentryTestCase(TestCase):
No files found
/home/byk/Code/getsentry/getsentry/getsentry/testutils/cases.py
<path>/home/byk/Code/getsentry/getsentry/getsentry/testutils/cases.py</path>
<type>file</type>
<content>
120: def message(field, expected, actual):
121: return f"{field} expected={expected} actual={actual}"
122:
123:
124: class GetsentryTestCase(TestCase):
125: # We need Django to flush all databases.
126: databases = "__all__"
127:
128: def _run_feature_check(self, handler, org):
129: """Check that an org has a feature enabled using a specific handler."""
130: feature = features.get(self.feature, org)
131: return handler.has(feature, actor=None)
132:
133: def create_detector_with_snuba_subscription(
134: self,
135: *,
136: is_error_type: bool = True,
137: detection_type: str | None = None,
138: status: int | None = None,
139: ) -> Detector:
140: """Create a Detector wired to a SnubaQuery subscription via a DataSource. ...
Found 1 matches /home/byk/Code/getsentry/getsentry/getsentry/testutils/cases.py: Line 36: from sentry.testutils.cases import (
Now let me look at the actual git log for the PR to see the full history of changes and what was actually committed:
Error: There is no jj repo in "." Hint: It looks like this is a git repo. You can create a jj repo backed by it by running this: jj git init
Error: There is no jj repo in "." Hint: It looks like this is a git repo. You can create a jj repo backed by it by running this: jj git init
29dde78bca fix(orbital): add dispatch_uid to event_accepted.connect 86e8df26c4 test(orbital): use empty string instead of None for unset ae1ed477fe test(orbital): exercise noop and startup paths via reimport d5a1d30bab fix(orbital): drop unused type: ignore on sendto 1140515902 fix(orbital): noop when socket unset, FQDN default DNS 6822013f33 fix(orbital): restore notify metrics, IP fallback, and Service DNS 673bf52f69 getsentry/sentry@f5465b21f3b2d263f8809223171185c1763901d5 513daac7a2 getsentry/sentry@c8e25a53acd5b1707cf1ed427bc888b6e7293dca 4d8cf1a2f6 getsentry/sentry@996740e2151646c68fafb5cdd4900c5eb9644ca1 2ff449d0d8 ref(replays): add start/end date range to delete_replays and per-day manifests for Ramp (#21331)
Error: There is no jj repo in "." Hint: It looks like this is a git repo. You can create a jj repo backed by it by running this: jj git init
OK so it's a git repo. Let me look at the full diff against the merge-base with master:
29dde78bca fix(orbital): add dispatch_uid to event_accepted.connect 86e8df26c4 test(orbital): use empty string instead of None for unset ae1ed477fe test(orbital): exercise noop and startup paths via reimport --- 29dde78bca5f95b3bf0acb6fe5bc6743d642a7a6 --- fix/orbital-volume-metrics-ip-dns
fatal: bad revision 'origin/main...HEAD'
* fix/orbital-volume-metrics-ip-dns master remotes/origin/0/ci/do_not_run/armenzg remotes/origin/11_14/db_errors/armenzg remotes/origin/11_21_getsentry_pydantic_v2 remotes/origin/Ajay/473-use-tier-upto remotes/origin/Ajay/5929-organization-contributors-table remotes/origin/Ajay/account-link-job remotes/origin/Ajay/disable-uptime-notifications remotes/origin/Ajay/uptake-card-auto-update-webhook remotes/origin/ArthurKnaus/feat/batch-span-attribute-fetching remotes/origin/ArthurKnaus/feat/enable-tracing-gocd-jobs remotes/origin/ArthurKnaus/feat/llm-generate-timeout remotes/origin/ArthurKnaus/feat/multi-project-low-value-spans remotes/origin/ArthurKnaus/fix/blocklist-unqueryable-span-attrs remotes/origin/ArthurKnaus/ref/align-detector-with-local-experiment remotes/origin/ArthurKnaus/ref/low-value-spans-llm-options remotes/origin/ArthurKnaus/ref/simplify-detector-logging remotes/origin/BIL-1025 ...
diff --git a/getsentry/conf/settings/cellsilo.py b/getsentry/conf/settings/cellsilo.py
index c8f5fcffc5..354e6f7359 100644
--- a/getsentry/conf/settings/cellsilo.py
+++ b/getsentry/conf/settings/cellsilo.py
@@ -2293,8 +2293,12 @@ SENTRY_OPTIONS["chart-rendering.storage.options"] = {
}
# Orbital — only runs in the US cell; elsewhere leave unset so no socket is opened.
+# Prefer the in-cluster Service DNS over a stale Internal LB IP (10.2.0.43 was
+# hardcoded since 2019 and no longer reaches all orbital pods reliably). The
+# trailing dot makes the name FQDN so pod-side `ndots:5` doesn't walk search
+# domains on every sendto (same pattern as twemproxy-…svc.cluster.local.).
if SENTRY_LOCAL_CELL == "us":
- ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER", "10.2.0.43:5556")
+ ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER", "orbital-udp.default.svc.cluster.local.:5556")
else:
ORBITAL_UDP_SERVER = env("ORBITAL_UDP_SERVER") or None
diff --git a/getsentry/receivers/orbital.py ...
Now let me check the actual test file to verify the current state:
<path>/home/byk/Code/getsentry/getsentry/tests/getsentry/receivers/test_orbital.py</path> <type>file</type> <content> 1: import importlib 2: from unittest import mock 3: from unittest.mock import MagicMock, patch 4: 5: from sentry.utils import json 6: 7: from getsentry import settings as getsentry_settings 8: from getsentry.receivers import orbital 9: from getsentry.testutils import GetsentryTestCase 10: 11: 12: class NotifyOrbitalTest(GetsentryTestCase): 13: def _outcomes(self, incr: MagicMock) -> list[str]: 14: return [call.kwargs["tags"]["outcome"] for call in incr.call_args_list] 15: 16: def test_notify_orbital_is_noop_when_udp_socket_unset(self) -> None: 17: """In non-US cells ORBITAL_UDP_SERVER is unset, so notify_orbital is 18: bound to a noop at module load and emits no per-event metrics. 19: 20: dev.py sets ORBITAL_UDP_SERVER to a valid value, so udp_socket is 21: always initialized in tests. ...
Now I have a comprehensive view. Let me also check the int(time() * 1000) — in the new code it's payload[2]. Let me check if the test verifies it.
Also, I want to check the test for test_no_socket_metric_emitted_at_startup_when_server_invalid — the assertEqual(self._outcomes(incr), ["no_socket"]) is inside the with patch(...) block. The importlib.reload(orbital) is also inside the patch block. So the metric is emitted during reload, and the patch captures it. Good.
But wait — there's a subtle issue. The with patch("getsentry.receivers.orbital.metrics.incr") patches metrics.incr in the orbital module. But during importlib.reload(orbital), the module is re-executed. The metrics.incr reference at the top of the file is from sentry.utils import metrics, so it's imported. When the module re-executes, it does from sentry.utils import metrics again, which gets the same metrics module. The patch("getsentry.receivers.orbital.metrics.incr") patches metrics.incr ...
No files found
Found 16 matches
/home/byk/Code/getsentry/getsentry/getsentry/receivers/orbital.py:
Line 43: def _notify_orbital(ip: Any, data: Any = None, **kwargs: Any) -> None:
Line 87: # Bind `notify_orbital` to the real impl when the socket is initialized; in
Line 92: notify_orbital = _notify_orbital
Line 95: def notify_orbital(*args: Any, **kwargs: Any) -> None:
Line 101: # would rebind `notify_orbital` to None and make it impossible to import/call directly
Line 105: event_accepted.connect(notify_orbital, weak=False, dispatch_uid="getsentry.receivers.orbital")
/home/byk/Code/getsentry/getsentry/tests/getsentry/receivers/test_orbital.py:
Line 16: def test_notify_orbital_is_noop_when_udp_socket_unset(self) -> None:
Line 17: """In non-US cells ORBITAL_UDP_SERVER is unset, so notify_orbital is
Line 31: orbital.notify_orbital("1.2.3.4", data={"platform": "python"})
Line 63: orbital._notify_orbital(None, ...
Good — notify_orbital is only used within the orbital module and its tests. No other module imports it. So the conditional binding is safe.
Now let me also check if the no_socket metric at startup is properly handled in the test environment. The test test_no_socket_metric_emitted_at_startup_when_server_invalid patches metrics.incr during the reload. But after the test, the patch is restored, and the real metrics.incr is used for subsequent operations. This is correct.
But there's a subtle issue: the test sets ORBITAL_UDP_SERVER = "not-a-valid-endpoint" and then reloads. During reload, the module tries to create the socket, fails, and calls metrics.incr(...). The patch captures this call. But the metrics.incr is called inside the except block, which is inside the if ORBITAL_UDP_SERVER: block. So the metric is only emitted if ORBITAL_UDP_SERVER is truthy. The test sets it to "not-a-valid-endpoint", which is truthy. Good. ...
Now I have enough context to write a thorough independent review. Let me also quickly verify one edge case about the BetterSignal.connect behavior with dispatch_uid to make sure my analysis is correct.
Found 12 matches /home/byk/Code/getsentry/sentry/src/sentry/signals.py: Line 58: def connect(self, receiver=None, *args, **kwargs): /home/byk/Code/getsentry/sentry/src/sentry/workflow_engine/defaults/workflows.py: Line 25: def connect_workflows_to_issue_stream( /home/byk/Code/getsentry/sentry/src/sentry/workflow_engine/endpoints/validators/utils.py: Line 156: def connect_workflows_to_detectors( Line 204: def connect_detectors_to_workflows( /home/byk/Code/getsentry/sentry/src/sentry/testutils/hybrid_cloud.py: Line 81: def connection_transaction_depth_above_watermark( Line 88: def connections_above_watermark(self) -> set[str]: /home/byk/Code/getsentry/sentry/src/sentry/net/http.py: Line 262: def connection_from_url(endpoint: str, **kw: Any): /home/byk/Code/getsentry/sentry/src/sentry/services/filestore/s3.py: Line 330: def connection(self): /home/byk/Code/getsentry/sentry/src/sentry/integrations/perforce/p4protocol/protocol.py: Line 261: ...
5.2.12 /home/byk/Code/getsentry/getsentry/.venv/lib/python3.13/site-packages/django/__init__.py
50: def connect(self, receiver, sender=None, weak=True, dispatch_uid=None):
<path>/home/byk/Code/getsentry/getsentry/.venv/lib/python3.13/site-packages/django/dispatch/dispatcher.py</path>
<type>file</type>
<content>
36: """
37: Create a new signal.
38: """
39: self.receivers = []
40: self.lock = threading.Lock()
41: self.use_caching = use_caching
42: # For convenience we create empty caches even if they are not used.
43: # A note about caching: if use_caching is defined, then for each
44: # distinct sender we cache the receivers that sender has in
45: # 'sender_receivers_cache'. The cache is cleaned when .connect() or
46: # .disconnect() is called and populated on send().
47: self.sender_receivers_cache = weakref.WeakKeyDictionary() if use_caching else {}
48: self._dead_receivers = False
49:
50: def connect(self, receiver, sender=None, weak=True, dispatch_uid=None):
51: """
52: Connect receiver to sender for signal. ...
<path>/home/byk/Code/getsentry/getsentry/.venv/lib/python3.13/site-packages/django/dispatch/dispatcher.py</path>
<type>file</type>
<content>
85:
86: # If DEBUG is on, check that we got a good receiver
87: if settings.configured and settings.DEBUG:
88: if not callable(receiver):
89: raise TypeError("Signal receivers must be callable.")
90: # Check for **kwargs
91: if not func_accepts_kwargs(receiver):
92: raise ValueError(
93: "Signal receivers must accept keyword arguments (**kwargs)."
94: )
95:
96: if dispatch_uid:
97: lookup_key = (dispatch_uid, _make_id(sender))
98: else:
99: lookup_key = (_make_id(receiver), _make_id(sender))
100:
101: is_async = iscoroutinefunction(receiver)
102:
103: if weak:
104: ref = weakref.ref
105: receiver_object = receiver
106: # Check for ...
Key insight from Django source (line 96-116):
if dispatch_uid:
lookup_key = (dispatch_uid, _make_id(sender))
else:
lookup_key = (_make_id(receiver), _make_id(sender))
...
with self.lock:
self._clear_dead_receivers()
if not any(r_key == lookup_key for r_key, _, _ in self.receivers):
self.receivers.append((lookup_key, receiver, is_async))
So with dispatch_uid, the lookup_key is (dispatch_uid, _make_id(sender)). The receiver is NOT part of the lookup key. This means:
dispatch_uid="X" and receiver=A: lookup_key=("X", sender_id). Not found, so appended. Signal has [("X", sender_id, A)].dispatch_uid="X" and receiver=B: lookup_key=("X", sender_id). FOUND, so NOT appended. Signal still has [("X", sender_id, A)].This means: the signal always has the FIRST receiver connected with that dispatch_uid, regardless of subsequent connects. The subsequent connects are silently dropped. ...
I now have a complete picture. Here is my independent review.
The PR restores the notify_orbital per-event metrics pipeline, adds an event.data.user.ip_address fallback when Relay's remote_addr is missing, fixes the US default DNS to use Service FQDN with a trailing dot, and avoids per-event no_socket metric spam in non-US cells. The six-commit history addresses each cursor/sentry-bugbot review comment in turn. Overall the code is correct and the cursor/bugbot comments are properly addressed. A handful of concerns remain around test coverage of the signal path, a subtle behavior change with rsplit for IPv6, and a few edge cases in the IP-fallback helper.
None. The code is functionally correct, the production behavior matches the PR description, and the cursor/sentry-bugbot comments are all addressed in subsequent commits.
test_notify_orbital_is_noop_when_udp_socket_unset does not actually exercise the signal pathFile: tests/getsentry/receivers/test_orbital.py:16-37
The test calls orbital.notify_orbital(...) directly, not through event_accepted.send_robust(...). Because the module was first imported with dev.py's valid ORBITAL_UDP_SERVER = "127.0.0.1:5556", the signal was already connected to the original _notify_orbital (the real impl) using dispatch_uid="getsentry.receivers.orbital". Django's Signal.connect (django/dispatch/dispatcher.py:96-116) uses (dispatch_uid, _make_id(sender)) as the lookup key and silently drops subsequent connects with the same key. The new noop is never placed on the signal — only the first-import real impl remains.
So in this test, event_accepted.send_robust(ip=..., data=...) would still call the real implementation and emit metrics, contradicting the docstring's claim that "the per-event handler doesn't emit a no_socket metric on every accepted ...
<observations> Date: Aug 4, 2026 * 🟡 (21:13) Grep across getsentry repo found 12 `connect` matches: sentry/signals.py:58 (BetterSignal.connect), workflow_engine/defaults/workflows.py:25 (connect_workflows_to_issue_stream), workflow_engine/endpoints/validators/utils.py:156/204 (connect_workflows_to_detectors, connect_detectors_to_workflows), testutils/hybrid_cloud.py:81/88 (connection_transaction…
<observations> Date: Aug 4, 2026 * 🟡 (21:13) Agent reasoning on test independence — tests in test_orbital.py are independent with their own try/finally blocks, so pytest reordering is safe. Each test restores state to original `ORBITAL_UDP_SERVER` with valid socket bound to `_notify_orbital` * 🟡 (21:13) Agent reasoning on `metrics.incr` at module load — `from sentry.utils import metrics` is a w…
<observations> Date: Aug 4, 2026 * 🟡 (21:12) Agent read getsentry/tests/getsentry/receivers/test_orbital.py (151 lines) — full test file for orbital receiver. Imports: `importlib`, `mock`, `MagicMock`, `patch`, `sentry.utils.json`, `getsentry.settings as getsentry_settings`, `getsentry.receivers.orbital`, `GetsentryTestCase`. Test class `NotifyOrbitalTest(GetsentryTestCase)` with helper `_outcom…
<observations> Date: Aug 4, 2026 * 🟡 (21:10) Agent read sentry/utils/geo.py (64 lines) — `geo_by_addr` is no-op by default (returns None), gets replaced by `_init_geoip` which opens `settings.GEOIP_PATH_MMDB` via maxminddb and returns `{country_code, region, city, latitude, longitude}`. Also has `_init_geoip_rust()` setting module-level `rust_geoip: None | GeoIpLookup` via `sentry_relay.processi…
<observations> Date: Aug 4, 2026 * 🟡 (21:09) Agent found ORBITAL_UDP_SERVER definitions across 6 settings files: defaults.py:1057 = `""`, dev.py:146 = `"127.0.0.1:5556"`, singletenant.py:1085 = `""`, controlsilo.py:835 = `env("ORBITAL_UDP_SERVER") or None`, cellsilo.py:2301-2303 = `env("ORBITAL_UDP_SERVER", "orbital-udp.default.svc.cluster.local.:5556")` for US cell else `env("ORBITAL_UDP_SERVER…
Date: Aug 4, 2026 * 🔴 (21:09) User stated that `ORBITAL_UDP_SERVER` is "always initialized in tests" — tests simulate the non-US path by reloading the module with the env var cleared * 🟡 (21:09) Agent reading getsentry/receivers/orbital.py (105 lines) — file contains: `udp_addr` parsing from `(host, port)` string, `notify_orbital` handler with fall-back to `user.ip_address` when Kafka remote_ad…
Date: Aug 4, 2026 * 🟡 (21:09) Agent starting independent code review of PR #21336 by reading actual files rather than relying solely on the diff — will examine cellsilo settings, getsentry settings module, signal infrastructure, test setup, and dev settings to understand no_socket/no-op interaction with real test setup
Date: Aug 4, 2026 * 🔴 (21:09) PR #21336 changes `getsentry/conf/settings/cellsilo.py`: US default ORBITAL_UDP_SERVER switches from hardcoded `10.2.0.43:5556` (stale since 2019) to `orbital-udp.default.svc.cluster.local.:5556`. Trailing dot makes FQDN so pod `ndots:5` doesn't walk search domains on sendto (same pattern as twemproxy-…svc.cluster.local.). Non-US cells still leave ORBITAL_UDP_SERVER…
<observations> Date: Aug 4, 2026 * 🔴 (21:08) User is performing an INDEPENDENT adversarial code review of PR #21336 in getsentry/getsentry ("fix(orbital): restore notify metrics, IP fallback, and Service DNS"). Branch: `fix/orbital-volume-metrics-ip-dns` * 🔴 (21:08) PR #21336 modifies 3 files: `getsentry/conf/settings/cellsilo.py`, `getsentry/receivers/orbital.py`, `tests/getsentry/receivers/te…