From 4d0d535cd4bf7f6786151c40fc59bee183568695 Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Wed, 9 Sep 2026 10:24:30 -0400 Subject: [PATCH 1/4] ref(api): Report client_kind attributes from every endpoint The caller attribution wiring lived in OrganizationEventsEndpointBase.convert_args, so only the ~44 events endpoints reported it. Move it to a single call site in Endpoint.dispatch, driven by an overridable client_kind_organization hook: the default reads the organization kwarg that every org-scoped base populates, and ProjectEndpoint reads it off the project it already select_relateds. Widens get_client_kind to accept RpcOrganization so control-silo organization endpoints are covered too. Coverage goes from ~44 endpoints to 300+, with no per-endpoint wiring. --- src/sentry/api/base.py | 25 ++++++++ src/sentry/api/bases/organization_events.py | 14 ----- src/sentry/api/bases/project.py | 11 ++++ src/sentry/api/client_kind.py | 16 +++-- tests/sentry/api/test_client_kind.py | 68 ++++++++++++++++++++- 5 files changed, 114 insertions(+), 20 deletions(-) diff --git a/src/sentry/api/base.py b/src/sentry/api/base.py index 442888a0baa6..94318440a50a 100644 --- a/src/sentry/api/base.py +++ b/src/sentry/api/base.py @@ -33,6 +33,7 @@ from sentry.analytics.events.release_set_commits import ReleaseSetCommitsLocalEvent from sentry.api.api_owners import ApiOwner from sentry.api.api_publish_status import ApiPublishStatus +from sentry.api.client_kind import set_client_kind_attributes from sentry.api.exceptions import ( INSUFFICIENT_SCOPE_ATTR, InsufficientScope, @@ -45,7 +46,9 @@ from sentry.auth.staff import has_staff_option from sentry.hybridcloud.apigateway.cell_request_resolvers import CellRequestResolver from sentry.middleware import is_frontend_request +from sentry.models.organization import Organization from sentry.organizations.absolute_url import generate_organization_url +from sentry.organizations.services.organization import RpcOrganization from sentry.ratelimits.config import DEFAULT_RATE_LIMIT_CONFIG, RateLimitConfig from sentry.seer import agent_token from sentry.silo.base import SiloLimit, SiloMode @@ -292,6 +295,24 @@ def build_cursor_link(self, request: HttpRequest, name: str, cursor: Cursor) -> def convert_args(self, request: Request, *args, **kwargs): return (args, kwargs) + def client_kind_organization( + self, request: Request, kwargs: dict[str, Any] + ) -> Organization | RpcOrganization | None: + """The organization whose `client_kind` opt-in governs this request, if any. + + Reads the `organization` that `convert_args` resolved, the kwarg every + organization-scoped base populates. Bases that resolve one some other way + override this -- `ProjectEndpoint` reads it off the project. Returning None + means the request goes unattributed, which is the right answer for an + endpoint with no organization in scope. + """ + organization = kwargs.get("organization") + # Type-checked rather than trusted: `kwargs` is whatever an arbitrary + # `convert_args` put there, and a non-organization would reach `features.has`. + if isinstance(organization, (Organization, RpcOrganization)): + return organization + return None + def permission_denied(self, request, message=None, code=None): """ Raise a specific superuser exception if the user can become superuser @@ -493,6 +514,10 @@ def dispatch(self, request: Request, *args, **kwargs) -> Response: (args, kwargs) = self.convert_args(request, *args, **kwargs) self.args = args self.kwargs = kwargs + + client_kind_organization = self.client_kind_organization(request, kwargs) + if client_kind_organization is not None: + set_client_kind_attributes(request, client_kind_organization) else: handler = self.http_method_not_allowed diff --git a/src/sentry/api/bases/organization_events.py b/src/sentry/api/bases/organization_events.py index d74c50e6db5d..23076c817c16 100644 --- a/src/sentry/api/bases/organization_events.py +++ b/src/sentry/api/bases/organization_events.py @@ -21,7 +21,6 @@ from sentry.api.base import CURSOR_LINK_HEADER from sentry.api.bases import NoProjects from sentry.api.bases.organization import FilterParamsDateNotNull, OrganizationEndpoint -from sentry.api.client_kind import set_client_kind_attributes from sentry.api.helpers.error_upsampling import ( are_any_projects_error_upsampled, convert_fields_for_upsampling, @@ -113,19 +112,6 @@ def resolve_axis_column( class OrganizationEventsEndpointBase(OrganizationEndpoint): owner = ApiOwner.DATA_BROWSING - def convert_args( - self, - request: Request, - *args: Any, - **kwargs: Any, - ) -> tuple[tuple[Any, ...], dict[str, Any]]: - (args, kwargs) = super().convert_args(request, *args, **kwargs) - # Runs after authentication, so the credential-based checks in - # `get_client_kind` see the resolved auth. Done here rather than in each - # handler so every events endpoint reports the caller the same way. - set_client_kind_attributes(request, kwargs["organization"]) - return (args, kwargs) - def has_feature(self, organization: Organization, request: Request) -> bool: return ( features.has("organizations:discover-basic", organization, actor=request.user) diff --git a/src/sentry/api/bases/project.py b/src/sentry/api/bases/project.py index c1f5e2b041af..8b677ed64f42 100644 --- a/src/sentry/api/bases/project.py +++ b/src/sentry/api/bases/project.py @@ -17,6 +17,7 @@ from sentry.api.utils import get_date_range_from_params from sentry.constants import ObjectStatus from sentry.exceptions import InvalidParams +from sentry.models.organization import Organization from sentry.models.project import Project from sentry.models.projectredirect import ProjectRedirect from sentry.utils.sdk import Scope, bind_organization_context @@ -218,6 +219,16 @@ def convert_args( kwargs["project"] = project return (args, kwargs) + def client_kind_organization( + self, request: Request, kwargs: dict[str, Any] + ) -> Organization | None: + """A project endpoint reports against its project's organization. + + No extra query: `convert_args` already `select_related`s the organization. + """ + project = kwargs.get("project") + return project.organization if project is not None else None + def get_filter_params( self, request: Request, project: Project, date_filter_optional: bool = False ) -> dict[str, Any]: diff --git a/src/sentry/api/client_kind.py b/src/sentry/api/client_kind.py index d4b466766a5d..8688219f89cb 100644 --- a/src/sentry/api/client_kind.py +++ b/src/sentry/api/client_kind.py @@ -25,6 +25,7 @@ from sentry.auth.system import is_system_auth from sentry.middleware import is_frontend_request from sentry.models.organization import Organization +from sentry.organizations.services.organization import RpcOrganization from sentry.seer.agent_token import is_agent_auth from sentry.utils.http import SEER_REFERRER_HEADER, get_mcp_client_family, is_mcp_request from sentry.utils.sdk import get_transaction_name_from_request @@ -101,7 +102,9 @@ def client_kind_scope(kind: ClientKind) -> Generator[None]: ) -def get_client_kind(request: Request, organization: Organization) -> ClientKind | None: +def get_client_kind( + request: Request, organization: Organization | RpcOrganization +) -> ClientKind | None: """Classify the caller of an API request. Returns ``None`` when the org has not opted in, so that a disabled org is @@ -184,12 +187,15 @@ def get_client_kind(request: Request, organization: Organization) -> ClientKind return ClientKind.UNKNOWN -def set_client_kind_attributes(request: Request, organization: Organization) -> None: +def set_client_kind_attributes( + request: Request, organization: Organization | RpcOrganization +) -> None: """Record who called the endpoint, on a span and on the enclosing transaction. - A no-op when the org has not opted into ``client_kind``. Wired into - ``OrganizationEventsEndpointBase.convert_args`` so every events endpoint - reports the same set of attributes without hand-wiring them per handler. + A no-op when the org has not opted into ``client_kind``. Called once from + ``Endpoint.dispatch``, for whichever organization + ``Endpoint.client_kind_organization`` resolves, so every endpoint reports the + same set of attributes without hand-wiring them per handler. """ client_kind = get_client_kind(request, organization) if client_kind is None: diff --git a/tests/sentry/api/test_client_kind.py b/tests/sentry/api/test_client_kind.py index 4e58e1ef8c8d..697e64c34ad1 100644 --- a/tests/sentry/api/test_client_kind.py +++ b/tests/sentry/api/test_client_kind.py @@ -9,6 +9,8 @@ from rest_framework.test import APIRequestFactory from sentry_conventions.attributes import ATTRIBUTE_NAMES +from sentry.api.base import Endpoint +from sentry.api.bases.project import ProjectEndpoint from sentry.api.client_kind import ( ATTRIBUTION_SPAN_OP, FEATURE_FLAG, @@ -21,9 +23,10 @@ ) from sentry.auth.services.auth import AuthenticatedToken from sentry.auth.system import SystemToken +from sentry.organizations.services.organization.serial import serialize_rpc_organization from sentry.seer.agent_token import AGENT_TOKEN_KIND from sentry.seer.endpoints.seer_rpc import SeerRpcSignatureAuthentication -from sentry.testutils.cases import TestCase +from sentry.testutils.cases import APITestCase, TestCase from sentry.utils.sdk import get_transaction_name_from_request EVENTS_PATH = "/api/0/organizations/my-org/events/" @@ -447,3 +450,66 @@ def test_declared_kind_is_recorded(self) -> None: set_client_kind_attributes(request, self.organization) assert sdk.set_tag.call_args_list == [mock.call("client_kind_test", "seer")] assert mock.call("client_kind_test", "seer") in sdk.set_attribute.call_args_list + + +class ClientKindOrganizationTest(TestCase): + """The resolver `Endpoint.dispatch` uses to find the org that governs the opt-in.""" + + def test_default_reads_the_organization_convert_args_resolved(self) -> None: + endpoint = Endpoint() + assert ( + endpoint.client_kind_organization(make_request(), {"organization": self.organization}) + is self.organization + ) + + def test_default_accepts_the_rpc_organization_a_control_silo_endpoint_resolves(self) -> None: + rpc_organization = serialize_rpc_organization(self.organization) + endpoint = Endpoint() + assert ( + endpoint.client_kind_organization(make_request(), {"organization": rpc_organization}) + is rpc_organization + ) + + def test_default_is_none_for_an_endpoint_with_no_organization(self) -> None: + assert Endpoint().client_kind_organization(make_request(), {}) is None + + def test_default_ignores_a_kwarg_that_is_not_an_organization(self) -> None: + """`kwargs` is whatever an arbitrary `convert_args` put there, so it is checked. + + Without this a base naming the kwarg differently would hand a slug to + `features.has` rather than simply reporting nothing. + """ + assert ( + Endpoint().client_kind_organization(make_request(), {"organization": "my-org"}) is None + ) + + def test_a_project_endpoint_reports_its_projects_organization(self) -> None: + endpoint = ProjectEndpoint() + assert ( + endpoint.client_kind_organization(make_request(), {"project": self.project}) + == self.organization + ) + + def test_a_project_endpoint_with_no_project_is_none(self) -> None: + assert ProjectEndpoint().client_kind_organization(make_request(), {}) is None + + +class DispatchWiringTest(APITestCase): + """Coverage reaches endpoints beyond the events base this started on.""" + + endpoint = "sentry-api-0-project-details" + + def test_a_project_endpoint_records_the_caller(self) -> None: + self.login_as(self.user) + with ( + self.feature(FEATURE_FLAG), + mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, + ): + self.get_success_response(self.organization.slug, self.project.slug) + assert mock.call("client_kind_test", "frontend") in sdk.set_tag.call_args_list + + def test_records_nothing_when_the_organization_has_not_opted_in(self) -> None: + self.login_as(self.user) + with mock.patch("sentry.api.client_kind.sentry_sdk") as sdk: + self.get_success_response(self.organization.slug, self.project.slug) + assert sdk.set_tag.call_args_list == [] From 37caa88d19ede734a343bee7a7eff83b834834a2 Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Wed, 9 Sep 2026 10:39:09 -0400 Subject: [PATCH 2/4] ref(api): Attribute team and issue endpoints too TeamEndpoint and GroupEndpoint resolve their organization off the related object rather than into the organization kwarg, so the default hook left 38 endpoints unattributed. Both already select_related the organization, so neither override costs a query. --- src/sentry/api/bases/team.py | 11 +++++++++++ src/sentry/issues/endpoints/bases/group.py | 10 ++++++++++ tests/sentry/api/test_client_kind.py | 22 ++++++++++++++++++++++ 3 files changed, 43 insertions(+) diff --git a/src/sentry/api/bases/team.py b/src/sentry/api/bases/team.py index 1b7abcadf615..a353284dc725 100644 --- a/src/sentry/api/bases/team.py +++ b/src/sentry/api/bases/team.py @@ -7,6 +7,7 @@ from sentry.api.base import Endpoint from sentry.api.exceptions import ResourceDoesNotExist +from sentry.models.organization import Organization from sentry.models.team import Team, TeamStatus from sentry.utils.sdk import bind_organization_context @@ -70,3 +71,13 @@ def convert_args( kwargs["team"] = team return (args, kwargs) + + def client_kind_organization( + self, request: Request, kwargs: dict[str, Any] + ) -> Organization | None: + """A team endpoint reports against its team's organization. + + No extra query: `convert_args` already `select_related`s the organization. + """ + team = kwargs.get("team") + return team.organization if team is not None else None diff --git a/src/sentry/issues/endpoints/bases/group.py b/src/sentry/issues/endpoints/bases/group.py index 0f312c5459ec..d9e127691946 100644 --- a/src/sentry/issues/endpoints/bases/group.py +++ b/src/sentry/issues/endpoints/bases/group.py @@ -110,6 +110,16 @@ def convert_args( return (args, kwargs) + def client_kind_organization( + self, request: Request, kwargs: dict[str, Any] + ) -> Organization | None: + """An issue endpoint reports against its group's organization. + + No extra query: `convert_args` already `select_related`s `project__organization`. + """ + group = kwargs.get("group") + return group.project.organization if group is not None else None + def get_external_issue_ids(self, group: Group) -> QuerySet[Any]: return GroupLink.objects.filter( project_id=group.project_id, group_id=group.id, linked_type=GroupLink.LinkedType.issue diff --git a/tests/sentry/api/test_client_kind.py b/tests/sentry/api/test_client_kind.py index 697e64c34ad1..b71d967057a3 100644 --- a/tests/sentry/api/test_client_kind.py +++ b/tests/sentry/api/test_client_kind.py @@ -11,6 +11,7 @@ from sentry.api.base import Endpoint from sentry.api.bases.project import ProjectEndpoint +from sentry.api.bases.team import TeamEndpoint from sentry.api.client_kind import ( ATTRIBUTION_SPAN_OP, FEATURE_FLAG, @@ -23,6 +24,7 @@ ) from sentry.auth.services.auth import AuthenticatedToken from sentry.auth.system import SystemToken +from sentry.issues.endpoints.bases.group import GroupEndpoint from sentry.organizations.services.organization.serial import serialize_rpc_organization from sentry.seer.agent_token import AGENT_TOKEN_KIND from sentry.seer.endpoints.seer_rpc import SeerRpcSignatureAuthentication @@ -493,6 +495,26 @@ def test_a_project_endpoint_reports_its_projects_organization(self) -> None: def test_a_project_endpoint_with_no_project_is_none(self) -> None: assert ProjectEndpoint().client_kind_organization(make_request(), {}) is None + def test_a_team_endpoint_reports_its_teams_organization(self) -> None: + endpoint = TeamEndpoint() + assert ( + endpoint.client_kind_organization(make_request(), {"team": self.team}) + == self.organization + ) + + def test_a_team_endpoint_with_no_team_is_none(self) -> None: + assert TeamEndpoint().client_kind_organization(make_request(), {}) is None + + def test_an_issue_endpoint_reports_its_groups_organization(self) -> None: + endpoint = GroupEndpoint() + assert ( + endpoint.client_kind_organization(make_request(), {"group": self.group}) + == self.organization + ) + + def test_an_issue_endpoint_with_no_group_is_none(self) -> None: + assert GroupEndpoint().client_kind_organization(make_request(), {}) is None + class DispatchWiringTest(APITestCase): """Coverage reaches endpoints beyond the events base this started on.""" From 9ceef94490fea106f1151c7417aa424e5b1af1cb Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Wed, 9 Sep 2026 11:36:02 -0400 Subject: [PATCH 3/4] ref(api): Check the client_kind opt-in at the call site Per review feedback: the organization was only ever needed for the feature flag, not for classification. Moving that check up to the dispatch call site lets get_client_kind and set_client_kind_attributes drop the organization parameter entirely, along with the Organization/RpcOrganization widening and both model imports. The client_kind_organization hook stays, so a base that resolves its organization some other way is still covered and plain Endpoint subclasses that populate the organization kwarg keep working without per-base wiring. At GA the hook and the features.has call delete together, leaving one unconditional call. Ordering matters here: the opt-in is checked before set_client_kind_attributes, so a client_kind_scope declaration cannot report for an org that never enabled the feature. That guarantee used to be enforced inside get_client_kind; test_a_declared_kind_does_not_bypass_the_opt_in now pins it at dispatch. --- src/sentry/api/base.py | 11 ++- src/sentry/api/client_kind.py | 34 ++++----- tests/sentry/api/test_client_kind.py | 72 +++++++++----------- tests/sentry/seer/endpoints/test_seer_rpc.py | 6 +- 4 files changed, 54 insertions(+), 69 deletions(-) diff --git a/src/sentry/api/base.py b/src/sentry/api/base.py index 94318440a50a..c3757be249ff 100644 --- a/src/sentry/api/base.py +++ b/src/sentry/api/base.py @@ -29,10 +29,11 @@ audit_logger = logging.getLogger("sentry.audit.api") api_access_logger = logging.getLogger("sentry.access.api") -from sentry import analytics, tsdb +from sentry import analytics, features, tsdb from sentry.analytics.events.release_set_commits import ReleaseSetCommitsLocalEvent from sentry.api.api_owners import ApiOwner from sentry.api.api_publish_status import ApiPublishStatus +from sentry.api.client_kind import FEATURE_FLAG as CLIENT_KIND_FEATURE_FLAG from sentry.api.client_kind import set_client_kind_attributes from sentry.api.exceptions import ( INSUFFICIENT_SCOPE_ATTR, @@ -515,9 +516,13 @@ def dispatch(self, request: Request, *args, **kwargs) -> Response: self.args = args self.kwargs = kwargs + # Resolved solely to check the opt-in; everything else is + # derived from the request. client_kind_organization = self.client_kind_organization(request, kwargs) - if client_kind_organization is not None: - set_client_kind_attributes(request, client_kind_organization) + if client_kind_organization is not None and features.has( + CLIENT_KIND_FEATURE_FLAG, client_kind_organization, actor=request.user + ): + set_client_kind_attributes(request) else: handler = self.http_method_not_allowed diff --git a/src/sentry/api/client_kind.py b/src/sentry/api/client_kind.py index 8688219f89cb..4e9d63e95656 100644 --- a/src/sentry/api/client_kind.py +++ b/src/sentry/api/client_kind.py @@ -20,12 +20,9 @@ from rest_framework.request import Request from sentry_conventions.attributes import ATTRIBUTE_NAMES -from sentry import features from sentry.auth.services.auth import AuthenticatedToken from sentry.auth.system import is_system_auth from sentry.middleware import is_frontend_request -from sentry.models.organization import Organization -from sentry.organizations.services.organization import RpcOrganization from sentry.seer.agent_token import is_agent_auth from sentry.utils.http import SEER_REFERRER_HEADER, get_mcp_client_family, is_mcp_request from sentry.utils.sdk import get_transaction_name_from_request @@ -102,18 +99,16 @@ def client_kind_scope(kind: ClientKind) -> Generator[None]: ) -def get_client_kind( - request: Request, organization: Organization | RpcOrganization -) -> ClientKind | None: +def get_client_kind(request: Request) -> ClientKind: """Classify the caller of an API request. - Returns ``None`` when the org has not opted in, so that a disabled org is - distinguishable from one whose traffic genuinely classifies as ``UNKNOWN``. - Otherwise never raises; unrecognized callers fall back to ``UNKNOWN``. - """ - if not features.has(FEATURE_FLAG, organization, actor=request.user): - return None + Never raises; unrecognized callers fall back to ``UNKNOWN``. + Says nothing about whether the caller's organization opted in -- ``FEATURE_FLAG`` + is checked by the caller, which is what holds the organization. Callers must + check it before reaching here, or a ``client_kind_scope`` declaration becomes a + way around the opt-in. + """ declared = _client_kind_override.get() if declared is not None: return declared @@ -187,19 +182,14 @@ def get_client_kind( return ClientKind.UNKNOWN -def set_client_kind_attributes( - request: Request, organization: Organization | RpcOrganization -) -> None: +def set_client_kind_attributes(request: Request) -> None: """Record who called the endpoint, on a span and on the enclosing transaction. - A no-op when the org has not opted into ``client_kind``. Called once from - ``Endpoint.dispatch``, for whichever organization - ``Endpoint.client_kind_organization`` resolves, so every endpoint reports the - same set of attributes without hand-wiring them per handler. + Called once from ``Endpoint.dispatch``, behind the opt-in check it makes for + whichever organization ``Endpoint.client_kind_organization`` resolves, so every + endpoint reports the same set of attributes without hand-wiring them per handler. """ - client_kind = get_client_kind(request, organization) - if client_kind is None: - return + client_kind = get_client_kind(request) client_host = get_client_host(request) user_agent = get_user_agent(request) diff --git a/tests/sentry/api/test_client_kind.py b/tests/sentry/api/test_client_kind.py index b71d967057a3..df60c3d91668 100644 --- a/tests/sentry/api/test_client_kind.py +++ b/tests/sentry/api/test_client_kind.py @@ -85,14 +85,8 @@ def api_token(*, application_id: int | None = None) -> AuthenticatedToken: class GetClientKindTest(TestCase): - def classify(self, request: Request) -> ClientKind | None: - with self.feature(FEATURE_FLAG): - return get_client_kind(request, self.organization) - - def test_returns_none_when_feature_is_disabled(self) -> None: - # A disabled org has to stay distinguishable from one that classifies as UNKNOWN. - with self.feature({FEATURE_FLAG: False}): - assert get_client_kind(make_request(), self.organization) is None + def classify(self, request: Request) -> ClientKind: + return get_client_kind(request) def test_session_auth_is_frontend(self) -> None: assert self.classify(make_request(user=session_user())) == ClientKind.FRONTEND @@ -264,25 +258,12 @@ def test_absent_header_is_none(self) -> None: class SetClientKindAttributesTest(TestCase): - def test_noop_when_feature_is_disabled(self) -> None: - request = make_request(auth=api_token(), user_agent="curl/8.7.1") - with ( - self.feature({FEATURE_FLAG: False}), - mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, - mock.patch("sentry.api.client_kind.start_span") as start_span, - ): - set_client_kind_attributes(request, self.organization) - sdk.set_tag.assert_not_called() - sdk.set_attribute.assert_not_called() - start_span.assert_not_called() - def test_records_kind_and_user_agent(self) -> None: request = make_request(auth=api_token(), user_agent="curl/8.7.1") with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) assert sdk.set_tag.call_args_list == [mock.call("client_kind_test", "script")] assert sdk.set_attribute.call_args_list == [ mock.call("client_kind_test", "script"), @@ -299,20 +280,18 @@ def test_records_client_host_for_mcp(self) -> None: }, ) with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) assert mock.call("client_host_test", "claude-code") in sdk.set_tag.call_args_list assert mock.call("client_host_test", "claude-code") in sdk.set_attribute.call_args_list def test_omits_user_agent_when_absent(self) -> None: request = make_request(auth=api_token()) with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) for call in sdk.set_attribute.call_args_list: assert call.args[0] != ATTRIBUTE_NAMES.USER_AGENT_ORIGINAL @@ -323,22 +302,20 @@ def test_records_for_the_internal_api_client_too(self) -> None: request = make_request(auth=api_token(), user_agent="curl/8.7.1") mark_from_api_client(request) with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, mock.patch("sentry.api.client_kind.start_span"), ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) assert sdk.set_tag.call_args_list == [mock.call("client_kind_test", "script")] class AttributionSpanTest(TestCase): def record(self, request: Request) -> tuple[Any, list[tuple[str, Any]]]: with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.start_span") as start_span, mock.patch("sentry.api.client_kind.set_span_data") as set_span_data, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) span = start_span.return_value.__enter__.return_value return start_span, [ call.args[1:] for call in set_span_data.call_args_list if call.args[0] is span @@ -401,9 +378,8 @@ def test_an_unmatched_path_collapses_onto_a_catch_all(self) -> None: class ClientKindScopeTest(TestCase): - def classify(self, request: Request) -> ClientKind | None: - with self.feature(FEATURE_FLAG): - return get_client_kind(request, self.organization) + def classify(self, request: Request) -> ClientKind: + return get_client_kind(request) def test_declared_kind_wins_over_a_signal_less_request(self) -> None: request = make_request(cookies=False) @@ -437,19 +413,13 @@ def test_nested_scopes_restore_the_outer_kind(self) -> None: assert self.classify(request) == ClientKind.MCP assert self.classify(request) == ClientKind.SEER - def test_org_opt_in_still_governs(self) -> None: - with client_kind_scope(ClientKind.SEER): - with self.feature({FEATURE_FLAG: False}): - assert get_client_kind(make_request(), self.organization) is None - def test_declared_kind_is_recorded(self) -> None: request = make_request(cookies=False) with ( - self.feature(FEATURE_FLAG), client_kind_scope(ClientKind.SEER), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) assert sdk.set_tag.call_args_list == [mock.call("client_kind_test", "seer")] assert mock.call("client_kind_test", "seer") in sdk.set_attribute.call_args_list @@ -532,6 +502,26 @@ def test_a_project_endpoint_records_the_caller(self) -> None: def test_records_nothing_when_the_organization_has_not_opted_in(self) -> None: self.login_as(self.user) - with mock.patch("sentry.api.client_kind.sentry_sdk") as sdk: + with ( + self.feature({FEATURE_FLAG: False}), + mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, + ): + self.get_success_response(self.organization.slug, self.project.slug) + assert sdk.set_tag.call_args_list == [] + + def test_a_declared_kind_does_not_bypass_the_opt_in(self) -> None: + """A declared caller must not also grant the organization's opt-in. + + The opt-in check moved out of `get_client_kind` and up to the dispatch call + site, so it is the ordering there -- not the function -- that now keeps a + `client_kind_scope` declaration from reporting for an org that never enabled + the feature. + """ + self.login_as(self.user) + with ( + self.feature({FEATURE_FLAG: False}), + client_kind_scope(ClientKind.SEER), + mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, + ): self.get_success_response(self.organization.slug, self.project.slug) assert sdk.set_tag.call_args_list == [] diff --git a/tests/sentry/seer/endpoints/test_seer_rpc.py b/tests/sentry/seer/endpoints/test_seer_rpc.py index ade3b9f3f97a..1339b3c6bb18 100644 --- a/tests/sentry/seer/endpoints/test_seer_rpc.py +++ b/tests/sentry/seer/endpoints/test_seer_rpc.py @@ -116,13 +116,13 @@ def test_get_organization_projects_registered_on_internal_rpc(self) -> None: def test_dispatch_declares_seer_as_the_client_kind(self) -> None: org = self.create_organization() - captured: list[ClientKind | None] = [] + captured: list[ClientKind] = [] def fake_method(**kwargs: Any) -> dict[str, Any]: nested = Request(APIRequestFactory().get("/")) nested.user = AnonymousUser() nested.auth = None - captured.append(get_client_kind(nested, org)) + captured.append(get_client_kind(nested)) return {"features": []} path = self._get_path("get_organization_features") @@ -151,7 +151,7 @@ def test_client_kind_scope_does_not_outlive_the_dispatch(self) -> None: nested = Request(APIRequestFactory().get("/")) nested.user = AnonymousUser() nested.auth = None - assert get_client_kind(nested, org) == ClientKind.UNKNOWN + assert get_client_kind(nested) == ClientKind.UNKNOWN def test_snuba_rate_limit_returns_429(self) -> None: """Test that SnubaRPCRateLimitExceeded returns 429 to Seer for retry.""" From f524659abbedd65437ff1ccd8d5e433f3ef9b8d7 Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Wed, 9 Sep 2026 11:47:39 -0400 Subject: [PATCH 4/4] ref(api): Read the client_kind organization off the request Every base that resolves an organization already assigns it to request._request.organization, and DRF proxies attribute lookups to the underlying HttpRequest, so dispatch can read it directly. access_log.py already reads it the same way. That removes the client_kind_organization hook and its three overrides -- no extension point on Endpoint for what is temporary scaffolding. kwargs is still consulted first, because the SentryApp bases populate only that and never touch the request. The hook's unit tests are replaced by real requests against organization, project, team and issue endpoints, which is the only honest way to pin coverage now that there is no seam. Removing the request fallback fails the project, team and issue cases. --- src/sentry/api/base.py | 29 ++--- src/sentry/api/bases/project.py | 11 -- src/sentry/api/bases/team.py | 11 -- src/sentry/issues/endpoints/bases/group.py | 10 -- tests/sentry/api/test_client_kind.py | 124 ++++++--------------- 5 files changed, 43 insertions(+), 142 deletions(-) diff --git a/src/sentry/api/base.py b/src/sentry/api/base.py index c3757be249ff..9bcec9deecdd 100644 --- a/src/sentry/api/base.py +++ b/src/sentry/api/base.py @@ -296,24 +296,6 @@ def build_cursor_link(self, request: HttpRequest, name: str, cursor: Cursor) -> def convert_args(self, request: Request, *args, **kwargs): return (args, kwargs) - def client_kind_organization( - self, request: Request, kwargs: dict[str, Any] - ) -> Organization | RpcOrganization | None: - """The organization whose `client_kind` opt-in governs this request, if any. - - Reads the `organization` that `convert_args` resolved, the kwarg every - organization-scoped base populates. Bases that resolve one some other way - override this -- `ProjectEndpoint` reads it off the project. Returning None - means the request goes unattributed, which is the right answer for an - endpoint with no organization in scope. - """ - organization = kwargs.get("organization") - # Type-checked rather than trusted: `kwargs` is whatever an arbitrary - # `convert_args` put there, and a non-organization would reach `features.has`. - if isinstance(organization, (Organization, RpcOrganization)): - return organization - return None - def permission_denied(self, request, message=None, code=None): """ Raise a specific superuser exception if the user can become superuser @@ -517,10 +499,13 @@ def dispatch(self, request: Request, *args, **kwargs) -> Response: self.kwargs = kwargs # Resolved solely to check the opt-in; everything else is - # derived from the request. - client_kind_organization = self.client_kind_organization(request, kwargs) - if client_kind_organization is not None and features.has( - CLIENT_KIND_FEATURE_FLAG, client_kind_organization, actor=request.user + # derived from the request. Both sources are conventions rather + # than contracts, so the result is type-checked before use. + organization = kwargs.get("organization") or getattr( + request, "organization", None + ) + if isinstance(organization, (Organization, RpcOrganization)) and features.has( + CLIENT_KIND_FEATURE_FLAG, organization, actor=request.user ): set_client_kind_attributes(request) else: diff --git a/src/sentry/api/bases/project.py b/src/sentry/api/bases/project.py index 8b677ed64f42..c1f5e2b041af 100644 --- a/src/sentry/api/bases/project.py +++ b/src/sentry/api/bases/project.py @@ -17,7 +17,6 @@ from sentry.api.utils import get_date_range_from_params from sentry.constants import ObjectStatus from sentry.exceptions import InvalidParams -from sentry.models.organization import Organization from sentry.models.project import Project from sentry.models.projectredirect import ProjectRedirect from sentry.utils.sdk import Scope, bind_organization_context @@ -219,16 +218,6 @@ def convert_args( kwargs["project"] = project return (args, kwargs) - def client_kind_organization( - self, request: Request, kwargs: dict[str, Any] - ) -> Organization | None: - """A project endpoint reports against its project's organization. - - No extra query: `convert_args` already `select_related`s the organization. - """ - project = kwargs.get("project") - return project.organization if project is not None else None - def get_filter_params( self, request: Request, project: Project, date_filter_optional: bool = False ) -> dict[str, Any]: diff --git a/src/sentry/api/bases/team.py b/src/sentry/api/bases/team.py index a353284dc725..1b7abcadf615 100644 --- a/src/sentry/api/bases/team.py +++ b/src/sentry/api/bases/team.py @@ -7,7 +7,6 @@ from sentry.api.base import Endpoint from sentry.api.exceptions import ResourceDoesNotExist -from sentry.models.organization import Organization from sentry.models.team import Team, TeamStatus from sentry.utils.sdk import bind_organization_context @@ -71,13 +70,3 @@ def convert_args( kwargs["team"] = team return (args, kwargs) - - def client_kind_organization( - self, request: Request, kwargs: dict[str, Any] - ) -> Organization | None: - """A team endpoint reports against its team's organization. - - No extra query: `convert_args` already `select_related`s the organization. - """ - team = kwargs.get("team") - return team.organization if team is not None else None diff --git a/src/sentry/issues/endpoints/bases/group.py b/src/sentry/issues/endpoints/bases/group.py index d9e127691946..0f312c5459ec 100644 --- a/src/sentry/issues/endpoints/bases/group.py +++ b/src/sentry/issues/endpoints/bases/group.py @@ -110,16 +110,6 @@ def convert_args( return (args, kwargs) - def client_kind_organization( - self, request: Request, kwargs: dict[str, Any] - ) -> Organization | None: - """An issue endpoint reports against its group's organization. - - No extra query: `convert_args` already `select_related`s `project__organization`. - """ - group = kwargs.get("group") - return group.project.organization if group is not None else None - def get_external_issue_ids(self, group: Group) -> QuerySet[Any]: return GroupLink.objects.filter( project_id=group.project_id, group_id=group.id, linked_type=GroupLink.LinkedType.issue diff --git a/tests/sentry/api/test_client_kind.py b/tests/sentry/api/test_client_kind.py index df60c3d91668..de84417a5229 100644 --- a/tests/sentry/api/test_client_kind.py +++ b/tests/sentry/api/test_client_kind.py @@ -9,9 +9,6 @@ from rest_framework.test import APIRequestFactory from sentry_conventions.attributes import ATTRIBUTE_NAMES -from sentry.api.base import Endpoint -from sentry.api.bases.project import ProjectEndpoint -from sentry.api.bases.team import TeamEndpoint from sentry.api.client_kind import ( ATTRIBUTION_SPAN_OP, FEATURE_FLAG, @@ -24,8 +21,6 @@ ) from sentry.auth.services.auth import AuthenticatedToken from sentry.auth.system import SystemToken -from sentry.issues.endpoints.bases.group import GroupEndpoint -from sentry.organizations.services.organization.serial import serialize_rpc_organization from sentry.seer.agent_token import AGENT_TOKEN_KIND from sentry.seer.endpoints.seer_rpc import SeerRpcSignatureAuthentication from sentry.testutils.cases import APITestCase, TestCase @@ -424,90 +419,48 @@ def test_declared_kind_is_recorded(self) -> None: assert mock.call("client_kind_test", "seer") in sdk.set_attribute.call_args_list -class ClientKindOrganizationTest(TestCase): - """The resolver `Endpoint.dispatch` uses to find the org that governs the opt-in.""" - - def test_default_reads_the_organization_convert_args_resolved(self) -> None: - endpoint = Endpoint() - assert ( - endpoint.client_kind_organization(make_request(), {"organization": self.organization}) - is self.organization - ) - - def test_default_accepts_the_rpc_organization_a_control_silo_endpoint_resolves(self) -> None: - rpc_organization = serialize_rpc_organization(self.organization) - endpoint = Endpoint() - assert ( - endpoint.client_kind_organization(make_request(), {"organization": rpc_organization}) - is rpc_organization - ) - - def test_default_is_none_for_an_endpoint_with_no_organization(self) -> None: - assert Endpoint().client_kind_organization(make_request(), {}) is None - - def test_default_ignores_a_kwarg_that_is_not_an_organization(self) -> None: - """`kwargs` is whatever an arbitrary `convert_args` put there, so it is checked. - - Without this a base naming the kwarg differently would hand a slug to - `features.has` rather than simply reporting nothing. - """ - assert ( - Endpoint().client_kind_organization(make_request(), {"organization": "my-org"}) is None - ) - - def test_a_project_endpoint_reports_its_projects_organization(self) -> None: - endpoint = ProjectEndpoint() - assert ( - endpoint.client_kind_organization(make_request(), {"project": self.project}) - == self.organization - ) - - def test_a_project_endpoint_with_no_project_is_none(self) -> None: - assert ProjectEndpoint().client_kind_organization(make_request(), {}) is None - - def test_a_team_endpoint_reports_its_teams_organization(self) -> None: - endpoint = TeamEndpoint() - assert ( - endpoint.client_kind_organization(make_request(), {"team": self.team}) - == self.organization - ) - - def test_a_team_endpoint_with_no_team_is_none(self) -> None: - assert TeamEndpoint().client_kind_organization(make_request(), {}) is None - - def test_an_issue_endpoint_reports_its_groups_organization(self) -> None: - endpoint = GroupEndpoint() - assert ( - endpoint.client_kind_organization(make_request(), {"group": self.group}) - == self.organization - ) - - def test_an_issue_endpoint_with_no_group_is_none(self) -> None: - assert GroupEndpoint().client_kind_organization(make_request(), {}) is None - - class DispatchWiringTest(APITestCase): - """Coverage reaches endpoints beyond the events base this started on.""" + """Coverage reaches endpoints beyond the events base this started on. - endpoint = "sentry-api-0-project-details" + Driven through real requests rather than a resolver seam: `Endpoint.dispatch` + reads the organization straight off `kwargs`/`request`, so the only honest way + to pin which endpoint families are covered is to call them. + """ - def test_a_project_endpoint_records_the_caller(self) -> None: + def setUp(self) -> None: + super().setUp() self.login_as(self.user) + + def tags_for(self, url: str, *, enabled: bool = True) -> list[Any]: with ( - self.feature(FEATURE_FLAG), + self.feature(FEATURE_FLAG if enabled else {FEATURE_FLAG: False}), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, ): - self.get_success_response(self.organization.slug, self.project.slug) - assert mock.call("client_kind_test", "frontend") in sdk.set_tag.call_args_list + assert self.client.get(url).status_code == 200 + return sdk.set_tag.call_args_list + + def test_an_organization_endpoint_records_the_caller(self) -> None: + url = f"/api/0/organizations/{self.organization.slug}/" + assert mock.call("client_kind_test", "frontend") in self.tags_for(url) + + def test_a_project_endpoint_records_the_caller(self) -> None: + url = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/" + assert mock.call("client_kind_test", "frontend") in self.tags_for(url) + + def test_a_team_endpoint_records_the_caller(self) -> None: + url = f"/api/0/teams/{self.organization.slug}/{self.team.slug}/" + assert mock.call("client_kind_test", "frontend") in self.tags_for(url) + + def test_an_issue_endpoint_records_the_caller(self) -> None: + # Team and issue endpoints resolve their organization off the related object + # rather than into an `organization` kwarg, so they are the families most + # likely to silently fall out of coverage. + url = f"/api/0/organizations/{self.organization.slug}/issues/{self.group.id}/" + assert mock.call("client_kind_test", "frontend") in self.tags_for(url) def test_records_nothing_when_the_organization_has_not_opted_in(self) -> None: - self.login_as(self.user) - with ( - self.feature({FEATURE_FLAG: False}), - mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, - ): - self.get_success_response(self.organization.slug, self.project.slug) - assert sdk.set_tag.call_args_list == [] + url = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/" + assert self.tags_for(url, enabled=False) == [] def test_a_declared_kind_does_not_bypass_the_opt_in(self) -> None: """A declared caller must not also grant the organization's opt-in. @@ -517,11 +470,6 @@ def test_a_declared_kind_does_not_bypass_the_opt_in(self) -> None: `client_kind_scope` declaration from reporting for an org that never enabled the feature. """ - self.login_as(self.user) - with ( - self.feature({FEATURE_FLAG: False}), - client_kind_scope(ClientKind.SEER), - mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, - ): - self.get_success_response(self.organization.slug, self.project.slug) - assert sdk.set_tag.call_args_list == [] + url = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/" + with client_kind_scope(ClientKind.SEER): + assert self.tags_for(url, enabled=False) == []