ref(discover): Restrict the fn argument of fn_span_count

[phacops]

Addresses VULN-2528.

What changed

fn_span_count in DiscoverDatasetConfig (src/sentry/search/events/datasets/discover.py) declared its second argument as SnQLStringArg("fn"). With no allowed_strings, StringArg.normalize() returns the caller's value unchanged, and that value is used as the name of the generated ClickHouse function.

This restricts the argument to an allowlist of the functions it makes sense to wrap a span count in:

FN_SPAN_COUNT_FUNCTIONS = [
    "identity", "sum", "avg", "min", "max", "median",
    "quantile(0.5)", "quantile(0.75)", "quantile(0.90)", "quantile(0.95)", "quantile(0.99)",
]

Anything outside that set is now rejected during query building (InvalidSearchQuery, i.e. a 400 from the events endpoints) instead of being forwarded.

The percentile entries are matched as written — the allowlist stays a plain string comparison, so quantile(0.42) is rejected like any other unlisted value. Passing a parameterised name through the function-name position is the same shape the shipped percentile functions already use (fields.py p95 is quantile(0.95); discover.py builds f"quantile({...})"), and FUNCTION_PATTERN + parse_arguments resolve fn_span_count("db", quantile(0.95)) to that argument unchanged.

Why an allowlist rather than removing the function

The function has no callers inside this repository — no Python usage, no static/ usage, no tests or fixtures, and no default discover/dashboard queries. Removing it was the first option considered.

However, Discover function names are part of the public query surface: they can be supplied directly through the events API and through query-authoring paths that build field expressions dynamically, neither of which leaves a reference in this repo. Sentry's own telemetry can't rule that usage out either — the events endpoints tag things like query.dataset and query.referrer but do not record the requested field values, so a successful call to this function leaves no searchable trace. (Searches over Sentry's own errors and logs for fn_span_count over 90 days return nothing, which rules out a crashing caller but not a working one.)

Removal and private=True are equally breaking for such a caller — both turn a working query into a 400. The allowlist is the only option that closes the argument injection while leaving legitimate use working, so that is what this does. The function is deliberately left non-private for the same reason.

identity is included as the passthrough wrapper consistent with this being declared via snql_column rather than snql_aggregate; it already exists as a function in its own right (sessions.py, and the legacy DiscoverFunction in fields.py).

Behaviour change for API consumers

A query passing an fn value outside the allowlist now gets a 400 (fn_span_count(...): fn argument invalid: string must be one of [...]) instead of having the value forwarded. Queries using any of the allowed values are unaffected.

Related audit

I checked the sibling SnQLFunction definitions in the same file and the other dataset configs for the same shape — a caller-supplied string reaching a ClickHouse function-name or column-name position without allowed_strings:

• modulo (discover.py) passes SnQLStringArg("column") into a Column(...) name position. It is already private=True, so it requires a functions_acl grant, and the intended column set isn't derivable from the code — left unchanged, flagged here for the owning team.
• Every other Function(args[...]) name position under src/sentry/search/events/datasets/ is fed by ConditionArg, which validates against a fixed list of conditions.
• The remaining SnQLStringArg arguments (count_op, trace_status_rate, http_response_rate, count_if, avg_if, to_other, …) are used in value positions, not name positions.

Testing

Added four tests to tests/sentry/search/events/builder/test_discover.py:

• test_fn_span_count_allowed_function — fn_span_count("db", sum) still resolves to the expected SnQL.
• test_fn_span_count_allowed_percentile — fn_span_count("db", quantile(0.95)) resolves to a quantile(0.95) wrapper.
• test_fn_span_count_disallowed_percentile — an unlisted level such as quantile(0.42) is rejected.
• test_fn_span_count_disallowed_function — any other unlisted value raises InvalidSearchQuery at query-build time rather than reaching ClickHouse.

I could not run anything locally: the environment this was prepared in has no Python virtualenv or installed dependencies (and no postgres/kafka/clickhouse devservices), so pytest could not run at all. The change was written against a static read of the code — tracing format_as_arguments → StringArg.normalize for the rejection path and error message, and FUNCTION_PATTERN / parse_arguments / get_function_alias_with_columns for the parsed arguments and expected aliases.

CI was therefore the first execution of these tests, and it is green on the current commit: all 18 backend test shards, backend typing, pre-commit lint, and the acceptance suite pass.

[linear-code]

VULN-2528