Skip to content

feat(allocation_policies): attach policies via sentry-options - #8387

Open
pbhandari wants to merge 15 commits into
masterfrom
prajjwalbhandari/eap-725-configure-allocation-policy-attachment-via-sentry-options
Open

feat(allocation_policies): attach policies via sentry-options#8387
pbhandari wants to merge 15 commits into
masterfrom
prajjwalbhandari/eap-725-configure-allocation-policy-attachment-via-sentry-options

Conversation

@pbhandari

@pbhandari pbhandari commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

What

Move allocation-policy configuration (which classes run for a ResourceIdentifier) from hardcoded construction onto a new sentry-option, allocation_policy.

  • AllocationPolicies each hardcode required_tenant_types, since those will require code changes anyway, hardcoding them will allow us to change this only in one place.
  • AllocationPolicys no longer have the following fields:
    • is_active => is_enforced is enough to disable it, or we can configure the values to have this be an empty dict. If the Policy exists and has flags tuned it's active.
    • default_config_overrides => 2 ways of overriding keys is confusing. Collating them into a single flow relying only on the **kwargs while keeping the ConfigurableComponent shape.
  • Currently we're not changing how each tenant configured allocation policies and am leaving configurable_component_overrides alone, my intent is to drop that config path completely and have this be configurable only via the proper path.

Part of EAP-725.

Behavior

  • Option is a map of resource.
  • Schema default is {}. Unset / empty falls back to PassthroughPolicy
  • StorageRouting.get_allocation_policies() reads EAP
  • SNQL storages with no YAML list fall through to the same helper (today: passthrough)

Tests

pytest tests/query/allocation_policies/test_attachment.py tests/web/rpc/v1/test_storage_routing.py::test_metrics_output tests/web/rpc/v1/routing_strategies/test_outcomes_based.py

@linear-code

linear-code Bot commented Aug 24, 2026

Copy link
Copy Markdown

EAP-725

@pbhandari

pbhandari commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Comment thread sentry-options/schemas/snuba/schema.json Outdated
Comment thread snuba/datasets/storage.py Outdated
Comment thread snuba/query/allocation_policies/__init__.py Outdated
Comment thread sentry-options/schemas/snuba/schema.json Outdated
Comment thread snuba/query/allocation_policies/__init__.py Outdated
@pbhandari
pbhandari marked this pull request as ready for review August 24, 2026 21:44
@pbhandari
pbhandari requested review from a team as code owners August 24, 2026 21:44
Comment thread snuba/query/allocation_policies/__init__.py Outdated
Comment thread snuba/query/allocation_policies/__init__.py Outdated
Comment thread snuba/query/allocation_policies/__init__.py
Comment on lines +368 to +370
# All routing strategies share allocation policies: they protect the same
# resource (EAP). The list is configured via allocation_policy.
return get_active_allocation_policies(ResourceIdentifier("EAP"))

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 code defaults to a PassthroughPolicy if the allocation_policy sentry-option is not configured, silently disabling EAP rate-limiting policies in production.
Severity: CRITICAL

Suggested Fix

To prevent silently disabling policies, the code should not fall back to a PassthroughPolicy if the allocation_policy option is missing for EAP. Instead, it should either log a prominent error and reject the query, or, more safely, default to the previous hardcoded policies (ConcurrentRateLimitAllocationPolicy, ReferrerGuardRailPolicy, BytesScannedRejectingPolicy) to maintain the existing safeguards. This ensures that a configuration oversight does not lead to a critical failure.

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: snuba/web/rpc/storage_routing/routing_strategies/storage_routing.py#L368-L370

Potential issue: The code replaces hardcoded EAP allocation policies with a system that
relies on the `allocation_policy` sentry-option. If this option is not configured in a
production environment before deployment, the system will default to a
`PassthroughPolicy`. This silently disables all previously active rate limiting,
byte-scanning limits, and concurrent query limits for EAP queries. This could allow an
unlimited number of EAP queries, potentially leading to cluster instability. The
fallback to `PassthroughPolicy` happens without any warning logs, making the issue
difficult to detect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Feature not bug.

Comment thread snuba/query/allocation_policies/__init__.py
Comment thread snuba/query/allocation_policies/__init__.py Outdated
@pbhandari
pbhandari force-pushed the prajjwalbhandari/eap-725-configure-allocation-policy-attachment-via-sentry-options branch from d2e969e to c109434 Compare August 25, 2026 19:34

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c109434. Configure here.

Comment thread snuba/query/allocation_policies/__init__.py

@phacops phacops left a comment

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.

We need to make sure these configurations get reloaded on each query and not just initialized when we start the server.

Comment thread snuba/query/allocation_policies/__init__.py
)

@property
def is_active(self) -> bool:

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.

Are we removing the option to measure the policy but not enforce it? I don't think we'd fully want that.

@pbhandari pbhandari Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My thinking isif the policy exists => it is "active". There is still the option to have a policy be un-enforced. But to imo "Policy exists in config but is configured to be entirely inert" doesn't seem wise.

The follow up PR allows us to explicitly "remove" policies instead to allow them to be neither enforced, nor active for specific overrides.

"default": {},
"description": "Override values for ConfigurableComponent (allocation policy and storage-routing strategy) configs, keyed by the fully-qualified config key '{resource}.{ClassName}.{config}' (parameterized configs append '|{param}:{value}|...', params sorted). Values are numbers cast to each config's declared numeric type (int/float) on read. All current ConfigurableComponent configs are numeric; a non-numeric config would require widening this type. Authoritative source for these configs; a key absent here falls back to the code default. Read-only at runtime and managed in sentry-options-automator. Migrated from the per-component runtime config of the same key."
},
"allocation_policy": {

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.

This should be plural.

See `_build_config_key()` for more info.
"""

required_tenant_types: frozenset[str] = frozenset()

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.

Why is this a property like this instead of a piece of configuration like before?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The follow up PR drops this completely. I just wanted to maintain a smaller blast radius for this change.

But I'm not sure this field is really a good idea tbh. I'm thinking all "tenant types" should be optional.

Comment on lines +635 to +645
AllocationPolicy.get_from_name(name).from_kwargs(
storage_key=resource_identifier.value,
**spec,
)
)
except InvalidConfigKeyError:
logger.warning(
"Unknown allocation policy %s for %s",
name,
resource_identifier.value,
)

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 get_active_allocation_policies function only catches InvalidConfigKeyError, leaving other exceptions from policy constructors (like ValueError) unhandled, which can crash request handlers.
Severity: HIGH

Suggested Fix

Broaden the except block in get_active_allocation_policies to catch other potential exceptions from policy initialization, such as ValueError and TypeError. Log the error and gracefully skip the malformed policy, which aligns with the apparent intent to degrade gracefully on bad configurations.

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: snuba/query/allocation_policies/__init__.py#L633-L645

Potential issue: The `get_active_allocation_policies` function initializes allocation
policies from configuration. Its error handling only catches `InvalidConfigKeyError`.
However, policy constructors, such as `CrossOrgQueryAllocationPolicy`, can raise other
exceptions like `ValueError` or `TypeError` if the configuration data is malformed
(e.g., a string instead of an integer for `max_threads`). An unhandled exception will
propagate to callers like `get_routing_decision` and `db_query`, potentially crashing
user-facing query routing and execution requests. This can occur with a misconfigured
`sentry-option` that passes schema validation but fails constructor validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants