Skip to content

feat(options): Add timing metric to options.get()#115762

Merged
kenzoengineer merged 4 commits into
masterfrom
kenjiang/di-1937-metrics-how-long-one-get-takes
May 19, 2026
Merged

feat(options): Add timing metric to options.get()#115762
kenzoengineer merged 4 commits into
masterfrom
kenjiang/di-1937-metrics-how-long-one-get-takes

Conversation

@kenzoengineer
Copy link
Copy Markdown
Member

@kenzoengineer kenzoengineer commented May 18, 2026

make sure to remove whitespace from the diff to view the real changes (minimal)

Wraps the OptionsManager.get() to establish baseline performance of the current sentry options system. We will compare this to the new sentry options system to ensure we don't regress. Also includes some tests to ensure the metrics function is being called.

This matches the pattern already used with features.has() (see src/sentry/features/manager.py line 280)

Assuming our metrics library is written well, there should be minimal overhead. We set the sample rate low (1%) to reduce metrics volume.

Add a `metrics.timer` around `OptionsManager.get()` to measure latency
of option lookups, tagged by resolution source (disk, store, default)
and region. This establishes baseline latency data ahead of the
sentry-options platform migration.

Refs DI-1934
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 18, 2026

DI-1937

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 18, 2026
except KeyError:
pass
else:
from sentry.utils import metrics
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we import metrics, we try to access settings.SENTRY_METRICS_SKIP_ALL_INTERNAL. If we move this import to the top, we do this before django is configured (and thus, django settings), causing a crash.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see line 23 of src/sentry/utils/metrics.py

Copy link
Copy Markdown
Member Author

@kenzoengineer kenzoengineer May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this is because options/manager.py is imported before django is configured in case that isn't clear).

moving the import down here means it'll only be imported when the application makes the call, long after we set up django

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for specifying 🙏

@kenzoengineer kenzoengineer marked this pull request as ready for review May 18, 2026 21:59
@kenzoengineer kenzoengineer requested a review from a team as a code owner May 18, 2026 21:59
@kenzoengineer kenzoengineer requested a review from a team as a code owner May 18, 2026 22:40
Comment thread src/sentry/options/manager.py
@kenzoengineer kenzoengineer requested a review from shellmayr May 18, 2026 23:17
Comment thread src/sentry/options/manager.py Outdated

with metrics.timer(
"options.store.get",
tags={"key": key, "region": settings.SENTRY_OPTIONS.get("system.region", "unknown")},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a system.region option/value. Perhaps you want to use settings.SENTRY_LOCAL_CELL instead? That setting will give you the cell/region name but will be None in control + single-tenants. To capture all of those deployment modes is more complex :(.

sentry_env = os.environ.get("CUSTOMER_ID", settings.SENTRY_LOCAL_CELL) or "unknown"
if SiloMode.get_current_mode() == SiloMode.CONTROL:
  sentry_env = SiloMode.CONTROL.name

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh thank you i missed this, I'm ok with having this block of code if it means I have metrics per region

@kenzoengineer kenzoengineer requested a review from markstory May 19, 2026 16:20
else:
from sentry.utils import metrics

sentry_env = os.environ.get("CUSTOMER_ID", settings.SENTRY_LOCAL_CELL) or "unknown"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The region metric tag incorrectly prioritizes the CUSTOMER_ID environment variable over SENTRY_LOCAL_CELL (SENTRY_REGION), leading to incorrect region tagging in metrics.
Severity: MEDIUM

Suggested Fix

Reverse the order of lookups for the region tag. Prioritize settings.SENTRY_LOCAL_CELL and use os.environ.get("CUSTOMER_ID") as the fallback, following the existing pattern in the codebase.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/sentry/options/manager.py#L299

Potential issue: The logic for determining the `region` metric tag is inverted. It
checks the `CUSTOMER_ID` environment variable before `settings.SENTRY_LOCAL_CELL` (which
is derived from `SENTRY_REGION`). In production environments where both variables are
set, such as Sentry's own deployment, the `region` tag will be incorrectly populated
with a customer ID (e.g., "sentry4sentry") instead of the actual region name. This will
render region-based analysis of timing metrics inaccurate. The established pattern in
the codebase is to check for the region first.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CUSTOMER_ID and LOCAL_CELL are mutually exclusive when we configure environments.

@kenzoengineer kenzoengineer merged commit 64b2c33 into master May 19, 2026
83 checks passed
@kenzoengineer kenzoengineer deleted the kenjiang/di-1937-metrics-how-long-one-get-takes branch May 19, 2026 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants