diff --git a/.github/workflows/scripts/compute-sentry-selected-tests.py b/.github/workflows/scripts/compute-sentry-selected-tests.py index 4066423106d1..f5a8c30fe83b 100644 --- a/.github/workflows/scripts/compute-sentry-selected-tests.py +++ b/.github/workflows/scripts/compute-sentry-selected-tests.py @@ -157,7 +157,7 @@ "tests/sentry/backup/test_validate.py", } -# Seer public-API matrix discovers PUBLIC mutations at collection time, so +# Seer public-API matrix discovers published mutations at collection time, so # endpoint module edits (including publish_status flips) need an explicit include. PUBLIC_API_MATRIX_TEST = "tests/sentry/seer/endpoints/test_organization_agent_token.py" PUBLIC_API_MATRIX_PATH_TRIGGERS: list[re.Pattern[str]] = [ diff --git a/src/sentry/api/api_publish_status.py b/src/sentry/api/api_publish_status.py index 2636ee6a046e..ba1fdb847fc5 100644 --- a/src/sentry/api/api_publish_status.py +++ b/src/sentry/api/api_publish_status.py @@ -6,6 +6,19 @@ class ApiPublishStatus(Enum): Used to track if an API is publicly documented """ - PUBLIC = "public" # stable API that is visible in public documentation - PRIVATE = "private" # any API that will not be published at any point - EXPERIMENTAL = "experimental" # API in development and will be published at some point + # A promotion ladder, least to most committed. Only the members for which + # `is_published` is true reach the OpenAPI spec; see sentry/apidocs/hooks.py. + PRIVATE = "private" # not published, and not intended to be + EXPERIMENTAL = "experimental" # not published; PUBLIC is intended, but nothing enforces that + PUBLIC_EXPERIMENTAL = "public_experimental" # published, but may still change incompatibly + PUBLIC = "public" # published; its attributes and their types are a stability commitment + + @property + def is_published(self) -> bool: + """Whether methods with this status are emitted into the public OpenAPI spec. + + Publication and stability are separate promises. Everything published is held + to the same documentation bar -- descriptions, a tag, a unique summary, a + declared response shape -- whether or not it also promises not to change. + """ + return self in (ApiPublishStatus.PUBLIC, ApiPublishStatus.PUBLIC_EXPERIMENTAL) diff --git a/src/sentry/api/endpoints/organization_trace_item_metrics.py b/src/sentry/api/endpoints/organization_trace_item_metrics.py index 460d5ec2479e..5e5d9d4bd1db 100644 --- a/src/sentry/api/endpoints/organization_trace_item_metrics.py +++ b/src/sentry/api/endpoints/organization_trace_item_metrics.py @@ -1,4 +1,4 @@ -from typing import Never, NotRequired, TypedDict +from typing import Never from drf_spectacular.utils import OpenApiParameter, extend_schema from rest_framework import serializers @@ -14,6 +14,10 @@ OrganizationTraceItemAttributesEndpointBase, adjust_start_end_window, ) +from sentry.api.endpoints.organization_trace_item_metrics_types import ( + TraceMetricContext, + TraceMetricItem, +) from sentry.api.paginator import ChainPaginator, GenericOffsetPaginator from sentry.api.utils import handle_query_errors from sentry.apidocs.constants import ( @@ -22,6 +26,7 @@ RESPONSE_NOT_FOUND, RESPONSE_UNAUTHORIZED, ) +from sentry.apidocs.examples.trace_item_metric_examples import TraceItemMetricExamples from sentry.apidocs.parameters import GlobalParams, OrganizationParams from sentry.apidocs.response_types import ValidationErrorResponse, as_validation_errors from sentry.apidocs.utils import inline_sentry_response_serializer @@ -37,7 +42,6 @@ METRIC_UNIT_ALIAS, ) from sentry.search.eap.occurrences.query_utils import build_escaped_term_filter -from sentry.search.eap.trace_metrics.types import TraceMetricType from sentry.search.eap.types import SearchResolverConfig from sentry.snuba.referrer import Referrer, is_valid_referrer from sentry.snuba.trace_metrics import TraceMetrics @@ -63,26 +67,6 @@ MAX_METRICS_PER_PAGE = 1000 -class TraceMetricContext(TypedDict): - brief: NotRequired[str] - # Longer-form notes, normalized to a list to match the attributes context - # shape (see TraceItemAttributeContext.details). - details: NotRequired[list[str]] - - -class TraceMetricItem(TypedDict): - name: str - type: TraceMetricType - unit: str | None - # The EAP aggregate declares an integer search type but the value arrives as - # a float, so declare what is actually emitted. - count: float - lastSeen: float | None - # Only present when `expand=context` is requested and the - # data-browsing-attribute-context feature is enabled. - context: NotRequired[TraceMetricContext] - - class OrganizationTraceItemMetricsSerializer(serializers.Serializer[Never]): query = serializers.CharField(required=False) expand = serializers.MultipleChoiceField(choices=["context"], required=False) @@ -159,7 +143,7 @@ def validate_sort(self, value: str) -> str: @cell_silo_endpoint class OrganizationTraceItemMetricsEndpoint(OrganizationTraceItemAttributesEndpointBase): publish_status = { - "GET": ApiPublishStatus.EXPERIMENTAL, + "GET": ApiPublishStatus.PUBLIC_EXPERIMENTAL, } owner = ApiOwner.DATA_BROWSING @@ -187,6 +171,7 @@ class OrganizationTraceItemMetricsEndpoint(OrganizationTraceItemAttributesEndpoi 403: RESPONSE_FORBIDDEN, 404: RESPONSE_NOT_FOUND, }, + examples=TraceItemMetricExamples.LIST_TRACE_METRICS, ) def get( self, request: Request, organization: Organization diff --git a/src/sentry/api/endpoints/organization_trace_item_metrics_types.py b/src/sentry/api/endpoints/organization_trace_item_metrics_types.py new file mode 100644 index 000000000000..783d53df61f1 --- /dev/null +++ b/src/sentry/api/endpoints/organization_trace_item_metrics_types.py @@ -0,0 +1,23 @@ +from typing import NotRequired, TypedDict + +from sentry.search.eap.trace_metrics.types import TraceMetricType + + +class TraceMetricContext(TypedDict): + brief: NotRequired[str] + # Longer-form notes, normalized to a list to match the attributes context + # shape (see TraceItemAttributeContext.details). + details: NotRequired[list[str]] + + +class TraceMetricItem(TypedDict): + name: str + type: TraceMetricType + unit: str | None + # The EAP aggregate declares an integer search type but the value arrives as + # a float, so declare what is actually emitted. + count: float + lastSeen: float | None + # Only present when `expand=context` is requested and the + # data-browsing-attribute-context feature is enabled. + context: NotRequired[TraceMetricContext] diff --git a/src/sentry/apidocs/examples/trace_item_metric_examples.py b/src/sentry/apidocs/examples/trace_item_metric_examples.py new file mode 100644 index 000000000000..97166967b399 --- /dev/null +++ b/src/sentry/apidocs/examples/trace_item_metric_examples.py @@ -0,0 +1,35 @@ +from drf_spectacular.utils import OpenApiExample + +from sentry.api.endpoints.organization_trace_item_metrics_types import TraceMetricItem + +TRACE_METRICS: list[TraceMetricItem] = [ + { + "name": "checkout.latency", + "type": "distribution", + "unit": "millisecond", + "count": 1432.0, + "lastSeen": 1735689600.0, + "context": { + "brief": "End-to-end latency of the checkout flow.", + "details": ["Recorded once per completed checkout, from cart submit to receipt."], + }, + }, + { + "name": "cart.items", + "type": "gauge", + "unit": None, + "count": 87.0, + "lastSeen": 1735686000.0, + }, +] + + +class TraceItemMetricExamples: + LIST_TRACE_METRICS = [ + OpenApiExample( + "List the trace metrics for an organization", + value=TRACE_METRICS, + response_only=True, + status_codes=["200"], + ) + ] diff --git a/src/sentry/apidocs/hooks.py b/src/sentry/apidocs/hooks.py index 8f48fbff45f1..7dee08d23191 100644 --- a/src/sentry/apidocs/hooks.py +++ b/src/sentry/apidocs/hooks.py @@ -43,13 +43,13 @@ class EndpointRegistryType(TypedDict): def __get_line_count_for_team_stats(team_stats: Mapping): """ Returns number of lines it takes to write ownership for each team. - For example returns 7 for: + For example returns 15 for: enterprise: { block_start: {line_number_for_enterprise}, public=[ExamplePublicEndpoint::GET], private=[ExamplePrivateEndpoint::GET], experimental=[ExampleExperimentalEndpoint::GET], - unknown=[ExampleUnknownEndpoint::GET] + public_experimental=[ExamplePublicExperimentalEndpoint::GET] } """ @@ -79,6 +79,9 @@ def __write_ownership_data(ownership_data: dict[ApiOwner, dict]): ApiPublishStatus.EXPERIMENTAL.value: sorted( ownership_data[team][ApiPublishStatus.EXPERIMENTAL] ), + ApiPublishStatus.PUBLIC_EXPERIMENTAL.value: sorted( + ownership_data[team][ApiPublishStatus.PUBLIC_EXPERIMENTAL] + ), } index += __get_line_count_for_team_stats(ownership_data[team]) dir = os.path.dirname(os.path.realpath(__file__)) @@ -104,9 +107,20 @@ class CustomGenerator(SchemaGenerator): # Collected during preprocessing, used in postprocessing _ENDPOINT_SERVERS: dict[str, list[dict[str, Any]]] = {} +# (path, lowercased method) pairs published as PUBLIC_EXPERIMENTAL. Preprocessing only +# filters endpoint tuples, so the marker has to be stamped onto the operation later. +_EXPERIMENTAL_OPERATIONS: set[tuple[str, str]] = set() + +# Prepended to the description of every PUBLIC_EXPERIMENTAL operation. The docs render +# operation descriptions as markdown but have no badge for `x-sentry-experimental`, so +# this is what actually warns a reader. Wording matches the note endpoints used to write +# by hand before the status existed. +EXPERIMENTAL_NOTICE = "**Experimental:** This API is under active development and may change." + def custom_preprocessing_hook(endpoints: Any) -> Any: # TODO: organize method, rename _ENDPOINT_SERVERS.clear() + _EXPERIMENTAL_OPERATIONS.clear() filtered = [] ownership_data: dict[ApiOwner, dict] = {} @@ -122,6 +136,7 @@ def custom_preprocessing_hook(endpoints: Any) -> Any: # TODO: organize method, ApiPublishStatus.PUBLIC: set(), ApiPublishStatus.PRIVATE: set(), ApiPublishStatus.EXPERIMENTAL: set(), + ApiPublishStatus.PUBLIC_EXPERIMENTAL: set(), } # Fail if endpoint is unowned @@ -143,13 +158,13 @@ def custom_preprocessing_hook(endpoints: Any) -> Any: # TODO: organize method, elif callback.view_class.publish_status: # endpoints that are documented via tooling - if ( - method in callback.view_class.publish_status - and callback.view_class.publish_status[method] is ApiPublishStatus.PUBLIC - ): + status = callback.view_class.publish_status.get(method) + if status is not None and status.is_published: # only pass declared public methods of the endpoint # to the rest of the OpenAPI build pipeline filtered.append((path, path_regex, method, callback)) + if status is ApiPublishStatus.PUBLIC_EXPERIMENTAL: + _EXPERIMENTAL_OPERATIONS.add((path, method.lower())) else: # if an endpoint doesn't have any registered public methods, don't check it. @@ -231,6 +246,17 @@ def custom_postprocessing_hook(result: Any, generator: Any, **kwargs: Any) -> An for method_info in result["paths"][path].values(): method_info["servers"] = servers + # Must run before _fix_issue_paths, which rewrites the path keys this is keyed on. + for path, method in _EXPERIMENTAL_OPERATIONS: + method_info = result["paths"].get(path, {}).get(method) + if method_info is not None: + method_info["x-sentry-experimental"] = True + description = method_info.get("description") + # Only prepend to an existing description; a missing one must still fail + # _check_description below rather than be silently satisfied here. + if description: + method_info["description"] = f"{EXPERIMENTAL_NOTICE}\n\n{description}" + _fix_issue_paths(result) _fix_nullable_enums(result) diff --git a/tests/apidocs/test_hooks.py b/tests/apidocs/test_hooks.py index bffe5640db88..28042ec41376 100644 --- a/tests/apidocs/test_hooks.py +++ b/tests/apidocs/test_hooks.py @@ -1,12 +1,18 @@ from typing import Any from unittest import TestCase +from unittest.mock import patch import pytest +from sentry.api.api_owners import ApiOwner +from sentry.api.api_publish_status import ApiPublishStatus from sentry.apidocs.hooks import ( _ENDPOINT_SERVERS, + _EXPERIMENTAL_OPERATIONS, + EXPERIMENTAL_NOTICE, _fix_nullable_enums, custom_postprocessing_hook, + custom_preprocessing_hook, ) from sentry.apidocs.utils import SentryApiBuildError @@ -54,6 +60,83 @@ def test_servers_applied_to_endpoint(self) -> None: assert "servers" not in processed["paths"]["/api/0/other/endpoint/"]["get"] +class PublishStatusFilterTest(TestCase): + """Only published statuses reach the OpenAPI pipeline, and PUBLIC_EXPERIMENTAL is marked.""" + + def setUp(self) -> None: + _EXPERIMENTAL_OPERATIONS.clear() + + def tearDown(self) -> None: + _ENDPOINT_SERVERS.clear() + _EXPERIMENTAL_OPERATIONS.clear() + + def _endpoint(self, path: str, status: ApiPublishStatus) -> tuple[Any, Any, str, Any]: + view_class = type( + "FakeEndpoint", + (), + { + "owner": ApiOwner.CRONS, + "publish_status": {"GET": status}, + "servers": None, + }, + ) + callback = type("FakeCallback", (), {"view_class": view_class}) + return (path, path, "GET", callback) + + @patch("sentry.apidocs.hooks.__write_ownership_data") + def test_only_published_statuses_pass_the_filter(self, _write_ownership: Any) -> None: + endpoints = [ + self._endpoint("/api/0/public/", ApiPublishStatus.PUBLIC), + self._endpoint("/api/0/public-experimental/", ApiPublishStatus.PUBLIC_EXPERIMENTAL), + self._endpoint("/api/0/experimental/", ApiPublishStatus.EXPERIMENTAL), + self._endpoint("/api/0/private/", ApiPublishStatus.PRIVATE), + ] + + filtered = custom_preprocessing_hook(endpoints) + + assert [path for path, _regex, _method, _cb in filtered] == [ + "/api/0/public/", + "/api/0/public-experimental/", + ] + assert _EXPERIMENTAL_OPERATIONS == {("/api/0/public-experimental/", "get")} + + def test_experimental_marker_stamped_on_operation(self) -> None: + _EXPERIMENTAL_OPERATIONS.add(("/api/0/public-experimental/", "get")) + + result = { + "components": {"schemas": {}}, + "paths": { + "/api/0/public-experimental/": { + "get": { + "tags": ["Events"], + "description": "An unstable endpoint", + "operationId": "get-unstable", + "parameters": [], + } + }, + "/api/0/public/": { + "get": { + "tags": ["Events"], + "description": "A stable endpoint", + "operationId": "get-stable", + "parameters": [], + } + }, + }, + } + + processed = custom_postprocessing_hook(result, None) + + experimental = processed["paths"]["/api/0/public-experimental/"]["get"] + assert experimental["x-sentry-experimental"] is True + assert "x-sentry-experimental" not in processed["paths"]["/api/0/public/"]["get"] + + # The docs render the description, not the marker, so the notice is the + # part a reader actually sees. + assert experimental["description"] == f"{EXPERIMENTAL_NOTICE}\n\nAn unstable endpoint" + assert processed["paths"]["/api/0/public/"]["get"]["description"] == "A stable endpoint" + + class SummaryUniquenessTest(TestCase): def _operation(self, summary: str) -> dict[str, Any]: return { diff --git a/tests/apidocs/test_operation_id_uniqueness.py b/tests/apidocs/test_operation_id_uniqueness.py index 02953ffac27d..79314c44c1ec 100644 --- a/tests/apidocs/test_operation_id_uniqueness.py +++ b/tests/apidocs/test_operation_id_uniqueness.py @@ -1,9 +1,9 @@ """Guards against duplicate ``operation_id`` values across ``@extend_schema`` decorators. Two operations sharing an ``operation_id`` produce an invalid OpenAPI document and -duplicate SDK function names. drf-spectacular's ``--fail-on-warn`` build only sees PUBLIC -operations, so it never catches a clash that involves a non-public method (e.g. a PUT/PATCH -pair on the same endpoint where only one is public). This test scans the source instead, so +duplicate SDK function names. drf-spectacular's ``--fail-on-warn`` build only sees published +operations, so it never catches a clash that involves an unpublished method (e.g. a PUT/PATCH +pair on the same endpoint where only one is published). This test scans the source instead, so it covers every ``@extend_schema`` regardless of publish status. (Summary uniqueness — which guards against docs-URL collisions — is enforced separately in diff --git a/tests/sentry/seer/endpoints/test_organization_agent_token.py b/tests/sentry/seer/endpoints/test_organization_agent_token.py index e10ec965067f..c0f1a48a94ea 100644 --- a/tests/sentry/seer/endpoints/test_organization_agent_token.py +++ b/tests/sentry/seer/endpoints/test_organization_agent_token.py @@ -23,7 +23,6 @@ from rest_framework.response import Response from rest_framework.test import APIClient -from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.endpoints.project_rules import ProjectRulesEndpoint from sentry.api.endpoints.seer_models import SEER_MODELS_CACHE_KEY from sentry.apidocs.hooks import CustomEndpointEnumerator @@ -130,11 +129,13 @@ def _public_get_endpoints() -> tuple[PublicGetEndpoint, ...]: discovered = enumerator._get_api_endpoints(enumerator.patterns, "") for path, _path_regex, method, callback in discovered: view = callback.view_class + status = view.publish_status.get(method) if ( method != "GET" or not path.startswith("/api/0/") or path.startswith("/api/0/{var}/") - or view.publish_status.get(method) is not ApiPublishStatus.PUBLIC + or status is None + or not status.is_published ): continue @@ -169,11 +170,13 @@ def _public_mutation_endpoints() -> tuple[PublicMutationEndpoint, ...]: discovered = enumerator._get_api_endpoints(enumerator.patterns, "") for path, _path_regex, method, callback in discovered: view = callback.view_class + status = view.publish_status.get(method) if ( method == "GET" or not path.startswith("/api/0/") or path.startswith("/api/0/{var}/") - or view.publish_status.get(method) is not ApiPublishStatus.PUBLIC + or status is None + or not status.is_published ): continue @@ -1280,6 +1283,7 @@ def _feature_flags( "OrganizationProfilingChunksEndpoint": "organizations:continuous-profiling", "OrganizationProfilingFlamegraphEndpoint": "organizations:profiling", "OrganizationTraceItemAttributesEndpoint": "organizations:visibility-explore-view", + "OrganizationTraceItemMetricsEndpoint": "organizations:visibility-explore-view", "ProjectProfilingProfileEndpoint": "organizations:profiling", } if feature := endpoint_flags.get(endpoint.endpoint_name): diff --git a/tests/tools/test_flake8_plugin.py b/tests/tools/test_flake8_plugin.py index 4b678a9dbc40..8984089249fb 100644 --- a/tests/tools/test_flake8_plugin.py +++ b/tests/tools/test_flake8_plugin.py @@ -777,7 +777,34 @@ def get(self) -> Response[BResponse]: ... ) -# --- S022: PUBLIC methods must declare a response shape --- +# --- S022: published methods must declare a response shape --- + + +def test_S022_public_experimental_bare_response_fires() -> None: + assert _resp("""\ +class E: + publish_status = {"GET": ApiPublishStatus.PUBLIC_EXPERIMENTAL} + def get(self) -> Response: ... +""") == ["3:S022"] + + +def test_S022_public_experimental_missing_annotation_fires() -> None: + assert _resp("""\ +class E: + publish_status = {"GET": ApiPublishStatus.PUBLIC_EXPERIMENTAL} + def get(self): ... +""") == ["3:S022"] + + +def test_S022_experimental_is_not_published_and_does_not_fire() -> None: + assert ( + _resp("""\ +class E: + publish_status = {"GET": ApiPublishStatus.EXPERIMENTAL} + def get(self) -> Response: ... +""") + == [] + ) def test_S022_public_bare_response_fires() -> None: diff --git a/tools/flake8_plugin.py b/tools/flake8_plugin.py index 24333425d6f2..a5670121646e 100644 --- a/tools/flake8_plugin.py +++ b/tools/flake8_plugin.py @@ -115,12 +115,12 @@ "Missing from the annotation: {}." ) S022_missing_msg = ( - "S022 PUBLIC endpoint methods must declare their response shape. This " + "S022 Published endpoint methods must declare their response shape. This " "method has no return annotation; use Response[YourTypedDict], a union of " "Response[T] arms, Response[None], or a non-DRF response type." ) S022_bare_msg = ( - "S022 PUBLIC endpoint methods must declare their response shape. Bare " + "S022 Published endpoint methods must declare their response shape. Bare " "`Response` opts the body out of type checking; use Response[YourTypedDict], " "a union of Response[T] arms, or Response[None]." ) @@ -234,6 +234,9 @@ def _collect_eap_suite_class_names(tree: ast.AST) -> set[str]: HTTP_METHODS = frozenset({"get", "post", "put", "patch", "delete", "head", "options"}) +# ApiPublishStatus members whose `is_published` is true. Mirrored by name because this +# plugin reads source with ast and must not import from src/sentry. +PUBLISHED_STATUSES = frozenset({"PUBLIC", "PUBLIC_EXPERIMENTAL"}) def publish_status(cls: ast.ClassDef) -> dict[str, str]: @@ -750,7 +753,7 @@ def _check_S021(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None: def _check_S022(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None: if not self._publish_status or node.name not in HTTP_METHODS: return - if self._publish_status.get(node.name.upper()) != "PUBLIC": + if self._publish_status.get(node.name.upper()) not in PUBLISHED_STATUSES: return if node.returns is None: self.errors.append((node.lineno, node.col_offset, S022_missing_msg))