diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 71f2d511d69d92..13781d0ce9ff22 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -806,6 +806,7 @@ tests/sentry/api/endpoints/test_organization_attribute_mappings.py @get /static/app/utils/theme/ @getsentry/design-engineering /static/app/components/commandPalette/ @getsentry/design-engineering /static/app/components/core/ @getsentry/design-engineering +/static/app/components/dnd/ @getsentry/design-engineering /static/app/components/pageFilters/ @getsentry/design-engineering /static/app/icons/ @getsentry/design-engineering /static/app/stories/ @getsentry/design-engineering diff --git a/.github/codeowners-coverage-baseline.txt b/.github/codeowners-coverage-baseline.txt index 1d3ee12af227dc..156f74abe3fb12 100644 --- a/.github/codeowners-coverage-baseline.txt +++ b/.github/codeowners-coverage-baseline.txt @@ -552,6 +552,7 @@ static/app/components/discover/transactionsList.spec.tsx static/app/components/discover/transactionsList.tsx static/app/components/discover/transactionsTable.tsx static/app/components/discoverButton.tsx +static/app/components/dnd/dragReorderButton.tsx static/app/components/dropdownButton.tsx static/app/components/dropdownMenu/footer.tsx static/app/components/dropdownMenu/index.spec.tsx diff --git a/.github/workflows/frontend-optional.yml b/.github/workflows/frontend-optional.yml index 24b1ed2f46a2b5..e018750130c229 100644 --- a/.github/workflows/frontend-optional.yml +++ b/.github/workflows/frontend-optional.yml @@ -25,8 +25,11 @@ jobs: testable_rules_changed: ${{ steps.changes.outputs.testable_rules_changed }} typecheckable_rules_changed: ${{ steps.changes.outputs.typecheckable_rules_changed }} frontend_all: ${{ steps.changes.outputs.frontend_all }} + merge_base: ${{ steps.merge_base.outputs.merge_base }} steps: - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 + with: + fetch-depth: 100 - name: Check for frontend file changes uses: dorny/paths-filter@0bc4621a3135347011ad047f9ecf449bf72ce2bd # v3.0.0 @@ -36,6 +39,88 @@ jobs: filters: .github/file-filters.yml list-files: shell + # On PRs, HEAD is the merge commit; its parents (HEAD^1, HEAD^2) are base and head. + # Merge base of those two is what Jest --changedSince needs. + # If merge base can't be computed or non-frontend files changed, output is empty + # and the optional Jest job will be skipped entirely. + - name: Get merge base for changedSince + id: merge_base + run: | + MERGE_BASE=$(git merge-base HEAD^1 HEAD^2 2>/dev/null) || true + if [ -n "$MERGE_BASE" ]; then + CHANGED=$(git diff --name-only "$MERGE_BASE" HEAD^2) + if echo "$CHANGED" | grep -qvE '^static/'; then + echo "Non-frontend file changed — skipping optional Jest" + MERGE_BASE="" + else + echo "Merge base: $MERGE_BASE (Jest will use --changedSince)" + fi + else + echo "Could not compute merge base — skipping optional Jest" + fi + echo "merge_base=${MERGE_BASE:-}" >> "$GITHUB_OUTPUT" + + # This job intentionally mirrors `frontend-jest-tests` in frontend.yml. + # Our intent is to try it out for a few weeks and see if it's stable. + frontend-jest-tests-changed-only: + if: >- + needs.files-changed.outputs.merge_base != '' && + (needs.files-changed.outputs.testable_rules_changed == 'true' || needs.files-changed.outputs.testable_modified == 'true') + needs: [files-changed] + name: Jest + # If you change the runs-on image, you must also change the runner in jest-balance.yml + # so that the balancer runs in the same environment as the tests. + runs-on: ubuntu-24.04 + timeout-minutes: 30 + strategy: + # This helps not having to run multiple jobs because one fails, thus, reducing resource usage + # and reducing the risk that one of many runs would turn red again (read: intermittent tests) + fail-fast: false + matrix: + # XXX: When updating this, make sure you also update CI_NODE_TOTAL. + instance: [0, 1, 2, 3] + + steps: + - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 + name: Checkout sentry + with: + # PRs need history so we can compute merge base for Jest --changedSince. + # 100 is an arbitrary depth that will get most reasonable PRs' commits. + fetch-depth: ${{ github.event_name == 'pull_request' && '100' || '1' }} + + - uses: ./.github/actions/setup-node-pnpm + + - name: Download jest-balance.json + id: download-artifact + uses: dawidd6/action-download-artifact@ac66b43f0e6a346234dd65d4d0c8fbb31cb316e5 # v11 + with: + workflow: 38531594 # jest-balancer.yml + workflow_conclusion: success # The conclusion of the workflow we're looking for + branch: master # The branch we're looking for + name: jest-balance.json # Artifact name + name_is_regexp: false + path: tests/js/test-balancer/ # Directory where to extract artifact(s), defaults to the current directory + search_artifacts: true # Search for the last workflow run whose stored the artifact we're looking for + if_no_artifact_found: warn # Can be one of: "fail", "warn", "ignore" + + - name: jest + env: + GITHUB_PR_SHA: ${{ github.event.pull_request.head.sha || github.sha }} + GITHUB_PR_REF: ${{ github.event.pull_request.head.ref || github.ref }} + # XXX: CI_NODE_TOTAL must be hardcoded to the length of strategy.matrix.instance. + # Otherwise, if there are other things in the matrix, using strategy.job-total + # wouldn't be correct. + CI_NODE_TOTAL: 4 + CI_NODE_INDEX: ${{ matrix.instance }} + # Disable testing-library from printing out any of of the DOM to + # stdout. No one actually looks through this in CI, they're just + # going to run it locally. + # + # This quiets up the logs quite a bit. + DEBUG_PRINT_LIMIT: 0 + MERGE_BASE: ${{ needs.files-changed.outputs.merge_base }} + run: pnpm run test-ci --forceExit + typescript-native: if: needs.files-changed.outputs.frontend_all == 'true' needs: files-changed diff --git a/eslint.config.ts b/eslint.config.ts index 3058c903290e26..d632d8b2b6da84 100644 --- a/eslint.config.ts +++ b/eslint.config.ts @@ -928,6 +928,7 @@ export default typescript.config([ name: 'files/jest related', files: [ 'tests/js/jest-pegjs-transform.js', + 'tests/js/sentry-test/jest-environment.js', 'tests/js/sentry-test/mocks/*', 'tests/js/sentry-test/loadFixtures.ts', 'tests/js/setup.ts', diff --git a/jest.config.ts b/jest.config.ts index b859f88264e964..04d12ff49b3685 100644 --- a/jest.config.ts +++ b/jest.config.ts @@ -69,7 +69,18 @@ let JEST_TESTS: string[] | undefined; // to reexec itself here if (CI && !process.env.JEST_LIST_TESTS_INNER) { try { - const stdout = execFileSync('pnpm', ['exec', 'jest', '--listTests', '--json'], { + const listTestArguments = ['exec', 'jest', '--listTests', '--json']; + + if (process.env.MERGE_BASE) { + console.log('MERGE_BASE detected:', process.env.MERGE_BASE); + listTestArguments.push( + '--changedSince', + process.env.MERGE_BASE, + '--passWithNoTests' + ); + } + + const stdout = execFileSync('pnpm', listTestArguments, { stdio: 'pipe', encoding: 'utf-8', env: {...process.env, JEST_LIST_TESTS_INNER: '1'}, @@ -108,6 +119,10 @@ function getTestsForGroup( allTests: ReadonlyArray, testStats: Record ): string[] { + if (allTests.length === 0) { + return []; + } + const speculatedSuiteDuration = Object.values(testStats).reduce((a, b) => a + b, 0); const targetDuration = speculatedSuiteDuration / nodeTotal; @@ -122,8 +137,13 @@ function getTestsForGroup( const tests = new Map(); const SUITE_P50_DURATION_MS = 1500; + const allTestsSet = new Set(allTests); + // First, iterate over all of the tests we have stats for. Object.entries(testStats).forEach(([test, duration]) => { + if (!allTestsSet.has(test)) { + return; + } if (duration <= 0) { throw new Error(`Test duration is <= 0 for ${test}`); } @@ -199,8 +219,8 @@ function getTestsForGroup( } } - if (!groups[nodeIndex]) { - throw new Error(`No tests found for node ${nodeIndex}`); + if (!groups[nodeIndex]?.length) { + return ['/__no_tests_for_this_shard__']; } return groups[nodeIndex].map(test => `/${test}`); } @@ -285,6 +305,7 @@ const config: Config.InitialOptions = { // window/cookies state. '@sentry/toolbar': '/tests/js/sentry-test/mocks/sentryToolbarMock.js', }, + passWithNoTests: !!process.env.MERGE_BASE, setupFiles: [ '/static/app/utils/silence-react-unsafe-warnings.ts', 'jest-canvas-mock', @@ -333,8 +354,7 @@ const config: Config.InitialOptions = { */ clearMocks: true, - // To disable the sentry jest integration, set this to 'jsdom' - testEnvironment: '@sentry/jest-environment/jsdom', + testEnvironment: '/tests/js/sentry-test/jest-environment.js', testEnvironmentOptions: { globalsCleanup: 'on', sentryConfig: { diff --git a/src/sentry/api/bases/organization.py b/src/sentry/api/bases/organization.py index fa1cc07e32f1c3..24bc36b1ed0a3d 100644 --- a/src/sentry/api/bases/organization.py +++ b/src/sentry/api/bases/organization.py @@ -706,14 +706,21 @@ def has_release_permission( organization: Organization | RpcOrganization, release: Release | None = None, project_ids: set[int] | None = None, + require_all_projects: bool = False, ) -> bool: """ Does the given request have permission to access this release, based on the projects to which the release is attached? - If the given request has an actor (user or ApiKey), cache the results - for a minute on the unique combination of actor,org,release, and project - ids. + By default, access is granted if the user has access to *any* project + on the release (suitable for reads). When require_all_projects=True, + the user must have access to *all* projects on the release (use for + mutations). Without this, a user with access to one project on a + multi-project release could modify or delete it, affecting projects + they cannot access. The all-projects check respects Open Membership + via has_global_access. + + Results are cached for 60s per actor/org/release/project-ids/mode. """ actor_id = None has_perms = None @@ -727,20 +734,23 @@ def has_release_permission( if requested_project_ids is None: requested_project_ids = self.get_requested_project_ids_unchecked(request) key = "release_perms:1:%s" % hash_values( - [actor_id, organization.id, release.id if release is not None else 0] + [ + actor_id, + organization.id, + release.id if release is not None else 0, + int(require_all_projects), + ] + sorted(requested_project_ids) ) has_perms = cache.get(key) if has_perms is None: projects = self.get_projects(request, organization, project_ids=project_ids) - # XXX(iambriccardo): The logic here is that you have access to this release if any of your projects - # associated with this release you have release permissions to. This is a bit of - # a problem because anyone can add projects to a release, so this check is easy - # to defeat. if release is not None: has_perms = ReleaseProject.objects.filter( release=release, project__in=projects ).exists() + if has_perms and require_all_projects: + has_perms = request.access.has_projects_access(list(release.projects.all())) else: has_perms = len(projects) > 0 diff --git a/src/sentry/api/endpoints/organization_trace_item_attributes.py b/src/sentry/api/endpoints/organization_trace_item_attributes.py index 414efb2b9cfac8..8aa23dd2a37ba4 100644 --- a/src/sentry/api/endpoints/organization_trace_item_attributes.py +++ b/src/sentry/api/endpoints/organization_trace_item_attributes.py @@ -12,12 +12,15 @@ TraceItemAttributeNamesResponse, TraceItemAttributeValuesRequest, ) -from sentry_protos.snuba.v1.request_common_pb2 import PageToken, RequestMeta +from sentry_protos.snuba.v1.request_common_pb2 import ( + PageToken, + RequestMeta, +) from sentry_protos.snuba.v1.request_common_pb2 import ( TraceItemType as ProtoTraceItemType, ) from sentry_protos.snuba.v1.trace_item_attribute_pb2 import AttributeKey -from sentry_protos.snuba.v1.trace_item_filter_pb2 import TraceItemFilter +from sentry_protos.snuba.v1.trace_item_filter_pb2 import ExistsFilter, OrFilter, TraceItemFilter from sentry import features, options from sentry.api.api_owners import ApiOwner @@ -840,10 +843,13 @@ def adjust_start_end_window(start_date: datetime, end_date: datetime) -> tuple[d return start_date, end_date -class OrganizationTraceItemAttributeValidateSerializer(serializers.Serializer): +class OrganizationTraceItemAttributeValidateQuerySerializer(serializers.Serializer): itemType = serializers.ChoiceField( [e.value for e in SupportedTraceItemType], required=True, source="item_type" ) + + +class OrganizationTraceItemAttributeValidateBodySerializer(serializers.Serializer): attributes = serializers.ListField( child=serializers.CharField(max_length=300), min_length=1, @@ -862,6 +868,72 @@ def serialize_type(search_type: constants.SearchType) -> str: return "number" +def _check_attributes_by_type( + meta: RequestMeta, + attr_type: AttributeKey.Type.ValueType, + names: list[str], +) -> set[tuple[AttributeKey.Type.ValueType, str]]: + """Check which typed attribute names exist in storage for the active window.""" + if not names: + return set() + + requested_names = set(names) + names_request = TraceItemAttributeNamesRequest( + meta=meta, + limit=10000, + type=attr_type, + intersecting_attributes_filter=TraceItemFilter( + or_filter=OrFilter( + filters=[ + TraceItemFilter( + exists_filter=ExistsFilter(key=AttributeKey(type=attr_type, name=name)) + ) + for name in requested_names + ] + ) + ), + ) + names_response = snuba_rpc.attribute_names_rpc(names_request) + return { + (attr_type, attribute.name) + for attribute in names_response.attributes + if attribute.name in requested_names + } + + +# We want to limit the number of threads to the number of attribute types to avoid overwhelming the RPC server. +MAX_ATTRIBUTE_VALIDATION_THREADS = 3 + + +def _check_attributes_exist( + resolver: SearchResolver, + item_type: SupportedTraceItemType, + attrs_by_type: dict[AttributeKey.Type.ValueType, list[str]], +) -> set[tuple[AttributeKey.Type.ValueType, str]]: + """Check which typed attribute internal names exist in storage.""" + if not attrs_by_type: + return set() + + meta = resolver.resolve_meta(referrer=Referrer.API_TRACE_ITEM_ATTRIBUTE_VALIDATE.value) + meta.trace_item_type = constants.SUPPORTED_TRACE_ITEM_TYPE_MAP.get( + item_type, ProtoTraceItemType.TRACE_ITEM_TYPE_SPAN + ) + + found: set[tuple[AttributeKey.Type.ValueType, str]] = set() + with ContextPropagatingThreadPoolExecutor( + thread_name_prefix="attr_validate", + max_workers=MAX_ATTRIBUTE_VALIDATION_THREADS, + ) as pool: + futures = [ + pool.submit(_check_attributes_by_type, meta, attr_type, names) + for attr_type, names in attrs_by_type.items() + ] + for future in futures: + found.update(future.result()) + + return found + + @cell_silo_endpoint class OrganizationTraceItemAttributeValidateEndpoint(OrganizationTraceItemAttributesEndpointBase): publish_status = { @@ -873,13 +945,16 @@ def post(self, request: Request, organization: Organization) -> Response: if not self.has_feature(organization, request): return Response(status=404) - serializer = OrganizationTraceItemAttributeValidateSerializer(data=request.data) + query_serializer = OrganizationTraceItemAttributeValidateQuerySerializer(data=request.GET) + if not query_serializer.is_valid(): + return Response(query_serializer.errors, status=400) + + serializer = OrganizationTraceItemAttributeValidateBodySerializer(data=request.data) if not serializer.is_valid(): return Response(serializer.errors, status=400) - validated = serializer.validated_data - item_type = SupportedTraceItemType(validated["item_type"]) - attribute_names: list[str] = validated["attributes"] + item_type = SupportedTraceItemType(query_serializer.validated_data["item_type"]) + attribute_names: list[str] = serializer.validated_data["attributes"] try: snuba_params = self.get_snuba_params(request, organization) @@ -897,17 +972,47 @@ def post(self, request: Request, organization: Organization) -> Response: ) results: dict[str, dict[str, Any]] = {} + # Collect unknown (user tag) attributes that need storage validation + unknown_attrs: list[tuple[str, Any]] = [] + for attr_name in attribute_names: try: resolved, _context = resolver.resolve_attribute(attr_name) - results[attr_name] = { - "valid": True, - "type": serialize_type(resolved.search_type), - } + if attr_name in definitions.contexts or attr_name in definitions.columns: + # Known column or virtual context — always valid + results[attr_name] = { + "valid": True, + "type": serialize_type(resolved.search_type), + } + else: + # User tag — need to verify it exists in storage + unknown_attrs.append((attr_name, resolved)) except InvalidSearchQuery as e: results[attr_name] = { "valid": False, "error": str(e), } + if unknown_attrs: + # Group by proto type because the storage check is keyed on + # (proto_type, internal_name) — the same display name can exist + # as both a string and a number attribute simultaneously. + attrs_by_type: dict[AttributeKey.Type.ValueType, list[str]] = {} + for _, resolved in unknown_attrs: + attrs_by_type.setdefault(resolved.proto_type, []).append(resolved.internal_name) + with handle_query_errors(): + existing = _check_attributes_exist(resolver, item_type, attrs_by_type) + + for attr_name, resolved in unknown_attrs: + if (resolved.proto_type, resolved.internal_name) in existing: + results[attr_name] = { + "valid": True, + "type": serialize_type(resolved.search_type), + } + else: + results[attr_name] = { + "valid": False, + "error": f"Unknown attribute: {attr_name}", + } + return Response({"attributes": results}) diff --git a/src/sentry/apidocs/examples/replay_examples.py b/src/sentry/apidocs/examples/replay_examples.py index f3950639384a70..83f29a13a2c20b 100644 --- a/src/sentry/apidocs/examples/replay_examples.py +++ b/src/sentry/apidocs/examples/replay_examples.py @@ -202,7 +202,7 @@ class ReplayExamples: GET_REPLAY_DELETION_JOBS = [ OpenApiExample( - "List replay deletion jobs", + "List replay batch deletion jobs", value={ "data": [ { @@ -225,7 +225,7 @@ class ReplayExamples: CREATE_REPLAY_DELETION_JOB = [ OpenApiExample( - "Create a replay deletion job", + "Create an async job to batch delete replay instances", value={ "data": { "id": 1, @@ -246,7 +246,7 @@ class ReplayExamples: GET_REPLAY_DELETION_JOB = [ OpenApiExample( - "Get a replay deletion job", + "Get a replay batch deletion job", value={ "data": { "id": 1, diff --git a/src/sentry/features/temporary.py b/src/sentry/features/temporary.py index 5fed3691df84d1..30d9f8e5afa01a 100644 --- a/src/sentry/features/temporary.py +++ b/src/sentry/features/temporary.py @@ -314,6 +314,8 @@ def register_temporary_features(manager: FeatureManager) -> None: manager.add("organizations:slack-staging-app", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) # Enable Seer Explorer in Slack via @mentions manager.add("organizations:seer-slack-explorer", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) + # Enable search query attribute validation + manager.add("organizations:search-query-attribute-validation", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) # Enable search query builder to support explicit boolean filters manager.add("organizations:search-query-builder-explicit-boolean-filters", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) # Enable search query builder input flow changes diff --git a/src/sentry/hybridcloud/apigateway/proxy.py b/src/sentry/hybridcloud/apigateway/proxy.py index aae730476e26b1..9f13ce9b8a6f2c 100644 --- a/src/sentry/hybridcloud/apigateway/proxy.py +++ b/src/sentry/hybridcloud/apigateway/proxy.py @@ -14,7 +14,7 @@ from django.http.response import HttpResponseBase from requests import Response as ExternalResponse from requests import request as external_request -from requests.exceptions import Timeout +from requests.exceptions import ConnectionError, Timeout from sentry import options from sentry.api.exceptions import RequestTimeout @@ -193,18 +193,22 @@ def proxy_cell_request(request: HttpRequest, cell: Cell, url_name: str) -> HttpR ) except Timeout: metrics.incr("apigateway.proxy.request_timeout", tags=metric_tags) - try: - if circuit_breaker is not None: - circuit_breaker.record_error() - except Exception: - logger.exception("Failed to record circuitbreaker failure") + if circuit_breaker is not None: + circuit_breaker.record_error() # remote silo timeout. Use DRF timeout instead raise RequestTimeout() + except ConnectionError: + metrics.incr("apigateway.proxy.connection_error", tags=metric_tags) + if circuit_breaker is not None: + circuit_breaker.record_error() + + raise - if resp.status_code >= 500 and circuit_breaker is not None: + if resp.status_code >= 502: metrics.incr("apigateway.proxy.request_failed", tags=metric_tags) - circuit_breaker.record_error() + if circuit_breaker is not None: + circuit_breaker.record_error() new_headers = clean_outbound_headers(resp.headers) resp.headers.clear() diff --git a/src/sentry/preprod/size_analysis/tasks.py b/src/sentry/preprod/size_analysis/tasks.py index f92815955a453b..29b8471283488b 100644 --- a/src/sentry/preprod/size_analysis/tasks.py +++ b/src/sentry/preprod/size_analysis/tasks.py @@ -74,198 +74,208 @@ def compare_preprod_artifact_size_analysis( return try: - if not artifact.commit_comparison: - logger.info( - "preprod.size_analysis.compare.artifact_no_commit_comparison", - extra={"artifact_id": artifact_id}, - ) - return - - comparisons: list[dict[str, PreprodArtifactSizeMetrics]] = [] - preprod_artifact_status_check_updates: set[int] = {artifact.id} - - # Create all comparisons with artifact as head - base_artifact = artifact.get_base_artifact_for_commit().first() - if base_artifact: - if artifact.build_configuration != base_artifact.build_configuration: - logger.info( - "preprod.size_analysis.compare.artifact_different_build_configurations", - extra={"head_artifact_id": artifact_id, "base_artifact_id": base_artifact.id}, + if artifact.commit_comparison: + comparisons: list[dict[str, PreprodArtifactSizeMetrics]] = [] + preprod_artifact_status_check_updates: set[int] = {artifact.id} + + # Create all comparisons with artifact as head + base_artifact = artifact.get_base_artifact_for_commit().first() + if base_artifact: + if artifact.build_configuration != base_artifact.build_configuration: + logger.info( + "preprod.size_analysis.compare.artifact_different_build_configurations", + extra={ + "head_artifact_id": artifact_id, + "base_artifact_id": base_artifact.id, + }, + ) + # Update the status check even though we can't compare to avoid leaving it in a loading state + create_preprod_status_check_task.apply_async( + kwargs={ + "preprod_artifact_id": artifact_id, + "caller": "compare_build_config_mismatch", + } + ) + return + + base_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter( + preprod_artifact_id=base_artifact.id, + preprod_artifact__project__organization_id=org_id, + preprod_artifact__project_id=project_id, + ).select_related("preprod_artifact", "preprod_artifact__mobile_app_info") + base_size_metrics = list(base_size_metrics_qs) + + head_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter( + preprod_artifact_id=artifact_id, + preprod_artifact__project__organization_id=org_id, + preprod_artifact__project_id=project_id, + ).select_related( + "preprod_artifact", + "preprod_artifact__mobile_app_info", + "preprod_artifact__commit_comparison", ) - # Update the status check even though we can't compare to avoid leaving it in a loading state - create_preprod_status_check_task.apply_async( - kwargs={ - "preprod_artifact_id": artifact_id, - "caller": "compare_build_config_mismatch", - } + head_size_metrics = list(head_size_metrics_qs) + + validation_result = can_compare_size_metrics(head_size_metrics, base_size_metrics) + if validation_result.can_compare: + base_metrics_map = build_size_metrics_map(base_size_metrics) + head_metrics_map = build_size_metrics_map(head_size_metrics) + + for key, base_metric in base_metrics_map.items(): + matching_head_size_metric = head_metrics_map.get(key) + if matching_head_size_metric: + logger.info( + "preprod.size_analysis.compare.create_comparison", + extra={ + "head_artifact_id": artifact_id, + "base_artifact_id": base_artifact.id, + }, + ) + comparisons.append( + { + "head_metric": matching_head_size_metric, + "base_metric": base_metric, + }, + ) + else: + logger.info( + "preprod.size_analysis.compare.no_matching_base_size_metric", + extra={ + "head_artifact_id": artifact_id, + "size_metric_id": base_metric.id, + }, + ) + else: + logger.info( + "preprod.size_analysis.compare.cannot_compare_size_metrics", + extra={ + "head_artifact_id": artifact_id, + "base_artifact_id": base_artifact.id, + "error_message": validation_result.error_message, + }, + ) + + # Also create comparisons with artifact as base + head_artifacts = artifact.get_head_artifacts_for_commit() + for head_artifact in head_artifacts: + if head_artifact.build_configuration != artifact.build_configuration: + logger.info( + "preprod.size_analysis.compare.head_artifact_different_build_configurations", + extra={ + "head_artifact_id": head_artifact.id, + "base_artifact_id": artifact_id, + }, + ) + continue + + head_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter( + preprod_artifact_id=head_artifact.id, + preprod_artifact__project__organization_id=org_id, + preprod_artifact__project_id=project_id, + ).select_related( + "preprod_artifact", + "preprod_artifact__mobile_app_info", + "preprod_artifact__commit_comparison", ) - return - - base_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter( - preprod_artifact_id=base_artifact.id, - preprod_artifact__project__organization_id=org_id, - preprod_artifact__project_id=project_id, - ).select_related("preprod_artifact", "preprod_artifact__mobile_app_info") - base_size_metrics = list(base_size_metrics_qs) - - head_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter( - preprod_artifact_id=artifact_id, - preprod_artifact__project__organization_id=org_id, - preprod_artifact__project_id=project_id, - ).select_related( - "preprod_artifact", - "preprod_artifact__mobile_app_info", - "preprod_artifact__commit_comparison", - ) - head_size_metrics = list(head_size_metrics_qs) + head_size_metrics = list(head_size_metrics_qs) + + base_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter( + preprod_artifact_id=artifact_id, + preprod_artifact__project__organization_id=org_id, + preprod_artifact__project_id=project_id, + ).select_related("preprod_artifact", "preprod_artifact__mobile_app_info") + base_size_metrics = list(base_size_metrics_qs) + + validation_result = can_compare_size_metrics(head_size_metrics, base_size_metrics) + if not validation_result.can_compare: + logger.info( + "preprod.size_analysis.compare.cannot_compare_size_metrics", + extra={ + "head_artifact_id": head_artifact.id, + "base_artifact_id": artifact_id, + "error_message": validation_result.error_message, + }, + ) + continue - validation_result = can_compare_size_metrics(head_size_metrics, base_size_metrics) - if validation_result.can_compare: - base_metrics_map = build_size_metrics_map(base_size_metrics) head_metrics_map = build_size_metrics_map(head_size_metrics) + base_metrics_map = build_size_metrics_map(base_size_metrics) - for key, base_metric in base_metrics_map.items(): - matching_head_size_metric = head_metrics_map.get(key) - if matching_head_size_metric: + for key, head_metric in head_metrics_map.items(): + matching_base_size_metric = base_metrics_map.get(key) + if matching_base_size_metric: logger.info( "preprod.size_analysis.compare.create_comparison", extra={ - "head_artifact_id": artifact_id, - "base_artifact_id": base_artifact.id, + "head_artifact_id": head_artifact.id, + "base_artifact_id": artifact.id, }, ) comparisons.append( - {"head_metric": matching_head_size_metric, "base_metric": base_metric}, + {"head_metric": head_metric, "base_metric": matching_base_size_metric}, ) + preprod_artifact_status_check_updates.add(head_artifact.id) else: logger.info( "preprod.size_analysis.compare.no_matching_base_size_metric", extra={ - "head_artifact_id": artifact_id, - "size_metric_id": base_metric.id, + "head_artifact_id": head_artifact.id, + "size_metric_id": head_metric.id, }, ) - else: - logger.info( - "preprod.size_analysis.compare.cannot_compare_size_metrics", - extra={ - "head_artifact_id": artifact_id, - "base_artifact_id": base_artifact.id, - "error_message": validation_result.error_message, - }, - ) - - # Also create comparisons with artifact as base - head_artifacts = artifact.get_head_artifacts_for_commit() - for head_artifact in head_artifacts: - if head_artifact.build_configuration != artifact.build_configuration: - logger.info( - "preprod.size_analysis.compare.head_artifact_different_build_configurations", - extra={"head_artifact_id": head_artifact.id, "base_artifact_id": artifact_id}, - ) - continue - - head_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter( - preprod_artifact_id=head_artifact.id, - preprod_artifact__project__organization_id=org_id, - preprod_artifact__project_id=project_id, - ).select_related( - "preprod_artifact", - "preprod_artifact__mobile_app_info", - "preprod_artifact__commit_comparison", - ) - head_size_metrics = list(head_size_metrics_qs) - base_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter( - preprod_artifact_id=artifact_id, - preprod_artifact__project__organization_id=org_id, - preprod_artifact__project_id=project_id, - ).select_related("preprod_artifact", "preprod_artifact__mobile_app_info") - base_size_metrics = list(base_size_metrics_qs) - - validation_result = can_compare_size_metrics(head_size_metrics, base_size_metrics) - if not validation_result.can_compare: - logger.info( - "preprod.size_analysis.compare.cannot_compare_size_metrics", - extra={ - "head_artifact_id": head_artifact.id, - "base_artifact_id": artifact_id, - "error_message": validation_result.error_message, - }, - ) - continue - - head_metrics_map = build_size_metrics_map(head_size_metrics) - base_metrics_map = build_size_metrics_map(base_size_metrics) + # Create PENDING comparison records in DB and run comparisons + with transaction.atomic(router.db_for_write(PreprodArtifactSizeComparison)): + for comp in comparisons: + head_metric = comp["head_metric"] + base_metric = comp["base_metric"] + comparison, created = PreprodArtifactSizeComparison.objects.get_or_create( + head_size_analysis=head_metric, + base_size_analysis=base_metric, + organization_id=org_id, + defaults={"state": PreprodArtifactSizeComparison.State.PENDING}, + ) - for key, head_metric in head_metrics_map.items(): - matching_base_size_metric = base_metrics_map.get(key) - if matching_base_size_metric: logger.info( - "preprod.size_analysis.compare.create_comparison", + "preprod.size_analysis.compare.running_comparison", extra={ - "head_artifact_id": head_artifact.id, - "base_artifact_id": artifact.id, + "head_metric_id": head_metric.id, + "base_metric_id": base_metric.id, + "comparison_created": created, }, ) - comparisons.append( - {"head_metric": head_metric, "base_metric": matching_base_size_metric}, - ) - preprod_artifact_status_check_updates.add(head_artifact.id) - else: - logger.info( - "preprod.size_analysis.compare.no_matching_base_size_metric", - extra={ - "head_artifact_id": head_artifact.id, - "size_metric_id": head_metric.id, - }, + _run_size_analysis_comparison(org_id, head_metric, base_metric) + + for artifact_id in preprod_artifact_status_check_updates: + # Update all artifact's status check with the new comparison + create_preprod_status_check_task.apply_async( + kwargs={ + "preprod_artifact_id": artifact_id, + "caller": "compare_completion", + } ) - # Create PENDING comparison records in DB and run comparisons - with transaction.atomic(router.db_for_write(PreprodArtifactSizeComparison)): - for comp in comparisons: - head_metric = comp["head_metric"] - base_metric = comp["base_metric"] - comparison, created = PreprodArtifactSizeComparison.objects.get_or_create( - head_size_analysis=head_metric, - base_size_analysis=base_metric, - organization_id=org_id, - defaults={"state": PreprodArtifactSizeComparison.State.PENDING}, - ) - - logger.info( - "preprod.size_analysis.compare.running_comparison", - extra={ - "head_metric_id": head_metric.id, - "base_metric_id": base_metric.id, - "comparison_created": created, - }, - ) - _run_size_analysis_comparison(org_id, head_metric, base_metric) - - for artifact_id in preprod_artifact_status_check_updates: - # Update all artifact's status check with the new comparison - create_preprod_status_check_task.apply_async( - kwargs={ - "preprod_artifact_id": artifact_id, - "caller": "compare_completion", - } - ) - - try: - artifact_type_name = PreprodArtifact.ArtifactType(artifact.artifact_type).name.lower() - except (ValueError, AttributeError, TypeError): - artifact_type_name = "unknown" - - e2e_size_analysis_compare_duration = timezone.now() - artifact.date_added - metrics.distribution( - "preprod.size_analysis.compare.results_e2e", - e2e_size_analysis_compare_duration.total_seconds(), - sample_rate=1.0, - tags={ - "artifact_type": artifact_type_name, - }, - ) + try: + artifact_type_name = PreprodArtifact.ArtifactType( + artifact.artifact_type + ).name.lower() + except (ValueError, AttributeError, TypeError): + artifact_type_name = "unknown" + + e2e_size_analysis_compare_duration = timezone.now() - artifact.date_added + metrics.distribution( + "preprod.size_analysis.compare.results_e2e", + e2e_size_analysis_compare_duration.total_seconds(), + sample_rate=1.0, + tags={ + "artifact_type": artifact_type_name, + }, + ) + else: + logger.info( + "preprod.size_analysis.compare.artifact_no_commit_comparison", + extra={"artifact_id": artifact_id}, + ) finally: send_size_analysis_webhook(artifact=artifact, organization_id=org_id) diff --git a/src/sentry/releases/endpoints/organization_release_assemble.py b/src/sentry/releases/endpoints/organization_release_assemble.py index e429323c9ffaf5..5c6e53bdfb13f9 100644 --- a/src/sentry/releases/endpoints/organization_release_assemble.py +++ b/src/sentry/releases/endpoints/organization_release_assemble.py @@ -36,7 +36,9 @@ def post(self, request: Request, organization, version) -> Response: except Release.DoesNotExist: raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist schema = { diff --git a/src/sentry/releases/endpoints/organization_release_details.py b/src/sentry/releases/endpoints/organization_release_details.py index b9a77e33d1685a..896b9a0dc42305 100644 --- a/src/sentry/releases/endpoints/organization_release_details.py +++ b/src/sentry/releases/endpoints/organization_release_details.py @@ -446,14 +446,12 @@ def put(self, request: Request, organization: Organization, version) -> Response scope.set_tag("failure_reason", "Release.DoesNotExist") raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): scope.set_tag("failure_reason", "no_release_permission") raise ResourceDoesNotExist - if not request.access.has_projects_access(list(projects)): - scope.set_tag("failure_reason", "no_access_to_all_projects") - raise ResourceDoesNotExist - serializer = OrganizationReleaseSerializer(data=request.data) if not serializer.is_valid(): @@ -562,10 +560,9 @@ def delete(self, request: Request, organization, version) -> Response: except Release.DoesNotExist: raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): - raise ResourceDoesNotExist - - if not request.access.has_projects_access(list(release.projects.all())): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist try: diff --git a/src/sentry/releases/endpoints/organization_release_file_details.py b/src/sentry/releases/endpoints/organization_release_file_details.py index fdb1739650e53b..aaef4eb8c36915 100644 --- a/src/sentry/releases/endpoints/organization_release_file_details.py +++ b/src/sentry/releases/endpoints/organization_release_file_details.py @@ -76,7 +76,9 @@ def put(self, request: Request, organization: Organization, version, file_id) -> except Release.DoesNotExist: raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist return self.update_releasefile(request, release, file_id) @@ -101,7 +103,9 @@ def delete(self, request: Request, organization, version, file_id) -> Response: except Release.DoesNotExist: raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist return self.delete_releasefile(release, file_id) diff --git a/src/sentry/releases/endpoints/organization_release_files.py b/src/sentry/releases/endpoints/organization_release_files.py index e50860f557ba6b..fc07d7a652efbb 100644 --- a/src/sentry/releases/endpoints/organization_release_files.py +++ b/src/sentry/releases/endpoints/organization_release_files.py @@ -103,7 +103,9 @@ def post(self, request: Request, organization, version) -> Response: logger = logging.getLogger("sentry.files") logger.info("organizationreleasefile.start") - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist return self.post_releasefile(request, release, logger) diff --git a/src/sentry/releases/endpoints/release_deploys.py b/src/sentry/releases/endpoints/release_deploys.py index d546fc772dd542..ca4dff93f9dd6c 100644 --- a/src/sentry/releases/endpoints/release_deploys.py +++ b/src/sentry/releases/endpoints/release_deploys.py @@ -207,9 +207,9 @@ def post(self, request: Request, organization, version) -> Response: ) raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): - # Logic here copied from `has_release_permission` (lightly edited for results to be more - # human-readable) + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): if request.user.is_authenticated: auth = f"user.id: {request.user.id}" elif request.auth is not None: diff --git a/src/sentry/replays/endpoints/project_replay_jobs_delete.py b/src/sentry/replays/endpoints/project_replay_jobs_delete.py index 9326af6caf8991..32dd4ccbbf127d 100644 --- a/src/sentry/replays/endpoints/project_replay_jobs_delete.py +++ b/src/sentry/replays/endpoints/project_replay_jobs_delete.py @@ -96,7 +96,7 @@ class ProjectReplayDeletionJobsIndexEndpoint(ProjectEndpoint): permission_classes = (ReplayDeletionJobPermission,) @extend_schema( - operation_id="List Replay Deletion Jobs", + operation_id="List Replay Batch-Deletion Jobs", parameters=[ GlobalParams.ORG_ID_OR_SLUG, GlobalParams.PROJECT_ID_OR_SLUG, @@ -131,7 +131,7 @@ def get(self, request: Request, project) -> Response: ) @extend_schema( - operation_id="Create a Replay Deletion Job", + operation_id="Create Replay Batch Deletion Job", parameters=[ GlobalParams.ORG_ID_OR_SLUG, GlobalParams.PROJECT_ID_OR_SLUG, @@ -200,7 +200,7 @@ class ProjectReplayDeletionJobDetailEndpoint(ProjectReplayEndpoint): permission_classes = (ReplayDeletionJobPermission,) @extend_schema( - operation_id="Get a Replay Deletion Job", + operation_id="Retrieve a Replay Batch-Deletion Job", parameters=[ GlobalParams.ORG_ID_OR_SLUG, GlobalParams.PROJECT_ID_OR_SLUG, diff --git a/src/sentry/seer/entrypoints/metrics.py b/src/sentry/seer/entrypoints/metrics.py index 912ba14eee5912..abe75ca3b8c311 100644 --- a/src/sentry/seer/entrypoints/metrics.py +++ b/src/sentry/seer/entrypoints/metrics.py @@ -9,6 +9,7 @@ class SeerOperatorInteractionType(StrEnum): OPERATOR_TRIGGER_AUTOFIX = "trigger_autofix" + OPERATOR_TRIGGER_EXPLORER = "trigger_explorer" OPERATOR_PROCESS_AUTOFIX_UPDATE = "process_autofix_update" OPERATOR_CACHE_POPULATE_PRE_AUTOFIX = "cache_populate_pre_autofix" OPERATOR_CACHE_POPULATE_POST_AUTOFIX = "cache_populate_post_autofix" @@ -19,6 +20,9 @@ class SeerOperatorInteractionType(StrEnum): ENTRYPOINT_ON_TRIGGER_AUTOFIX_ALREADY_EXISTS = "entrypoint_on_trigger_autofix_already_exists" ENTRYPOINT_CREATE_AUTOFIX_CACHE_PAYLOAD = "entrypoint_create_autofix_cache_payload" ENTRYPOINT_ON_AUTOFIX_UPDATE = "entrypoint_on_autofix_update" + ENTRYPOINT_ON_TRIGGER_EXPLORER = "entrypoint_on_trigger_explorer" + ENTRYPOINT_CREATE_EXPLORER_CACHE_PAYLOAD = "entrypoint_create_explorer_cache_payload" + ENTRYPOINT_ON_EXPLORER_UPDATE = "entrypoint_on_explorer_update" OPERATOR_PROCESS_EXPLORER_COMPLETION = "process_explorer_completion" OPERATOR_CACHE_SET_EXPLORER = "cache_set_explorer" OPERATOR_CACHE_GET_EXPLORER = "cache_get_explorer" diff --git a/src/sentry/seer/entrypoints/operator.py b/src/sentry/seer/entrypoints/operator.py index 9bf789887814ed..0d1e3062633d4c 100644 --- a/src/sentry/seer/entrypoints/operator.py +++ b/src/sentry/seer/entrypoints/operator.py @@ -28,9 +28,15 @@ autofix_entrypoint_registry, explorer_entrypoint_registry, ) -from sentry.seer.entrypoints.types import SeerAutofixEntrypoint, SeerEntrypointKey +from sentry.seer.entrypoints.types import ( + SeerAutofixEntrypoint, + SeerEntrypointKey, + SeerExplorerEntrypoint, +) +from sentry.seer.explorer.client import SeerExplorerClient from sentry.seer.explorer.client_models import SeerRunState from sentry.seer.explorer.on_completion_hook import ExplorerOnCompletionHook +from sentry.seer.models import SeerPermissionError from sentry.seer.seer_setup import has_seer_access from sentry.sentry_apps.metrics import SentryAppEventType from sentry.tasks.base import instrumented_task @@ -441,6 +447,116 @@ def trigger_autofix_legacy( ) +class SeerExplorerOperator[CachePayloadT]: + """ + A class that connects to entrypoint implementations and runs Explorer operations for Seer. + It does this to ensure all entrypoints have consistent behavior and responses. + """ + + def __init__(self, entrypoint: SeerExplorerEntrypoint[CachePayloadT]): + self.entrypoint = entrypoint + + def trigger_explorer( + self, + *, + organization: Organization, + user: User | RpcUser | None, + prompt: str, + on_page_context: str | None = None, + category_key: str, + category_value: str, + ) -> int | None: + """ + Start or continue an Explorer run and return the run_id. + If a run exists for this category (e.g. slack thread), continues it; otherwise starts new. + Uses the entrypoint's Explorer callbacks for success/error handling. + """ + event_lifecycle = SeerOperatorEventLifecycleMetric( + interaction_type=SeerOperatorInteractionType.OPERATOR_TRIGGER_EXPLORER, + entrypoint_key=self.entrypoint.key, + ) + + with event_lifecycle.capture() as lifecycle: + lifecycle.add_extras( + { + "category_key": category_key, + "category_value": category_value, + } + ) + + try: + # RpcUser is not in SeerExplorerClient's type signature but works at runtime + client = SeerExplorerClient( + organization=organization, + user=user, + category_key=category_key, + category_value=category_value, + on_completion_hook=SeerOperatorCompletionHook, + is_interactive=True, + enable_coding=False, + ) + except SeerPermissionError as e: + with SeerOperatorEventLifecycleMetric( + interaction_type=SeerOperatorInteractionType.ENTRYPOINT_ON_TRIGGER_EXPLORER, + entrypoint_key=self.entrypoint.key, + ).capture(assume_success=False): + self.entrypoint.on_trigger_explorer_error(error=str(e)) + lifecycle.record_failure(failure_reason=e) + return None + + try: + existing_runs = client.get_runs( + category_key=category_key, + category_value=category_value, + limit=1, + only_current_user=False, + ) + + if existing_runs: + run_id = client.continue_run( + run_id=existing_runs[0].run_id, + prompt=prompt, + on_page_context=on_page_context, + ) + lifecycle.add_extra("continued", "true") + else: + run_id = client.start_run( + prompt=prompt, + on_page_context=on_page_context, + ) + lifecycle.add_extra("continued", "false") + except Exception as e: + with SeerOperatorEventLifecycleMetric( + interaction_type=SeerOperatorInteractionType.ENTRYPOINT_ON_TRIGGER_EXPLORER, + entrypoint_key=self.entrypoint.key, + ).capture(assume_success=False): + self.entrypoint.on_trigger_explorer_error(error="An unexpected error occurred") + lifecycle.record_failure(failure_reason=e) + return None + + lifecycle.add_extra("run_id", str(run_id)) + + with SeerOperatorEventLifecycleMetric( + interaction_type=SeerOperatorInteractionType.ENTRYPOINT_ON_TRIGGER_EXPLORER, + entrypoint_key=self.entrypoint.key, + ).capture(): + self.entrypoint.on_trigger_explorer_success(run_id=run_id) + + with SeerOperatorEventLifecycleMetric( + interaction_type=SeerOperatorInteractionType.ENTRYPOINT_CREATE_EXPLORER_CACHE_PAYLOAD, + entrypoint_key=self.entrypoint.key, + ).capture(): + cache_payload = self.entrypoint.create_explorer_cache_payload() + + SeerOperatorExplorerCache.set( + entrypoint_key=str(self.entrypoint.key), + run_id=run_id, + cache_payload=cache_payload, + ) + + return run_id + + @instrumented_task( name="sentry.seer.entrypoints.operator.process_autofix_updates", namespace=seer_tasks, @@ -696,8 +812,22 @@ def execute(cls, organization: Organization, run_id: int) -> None: if not cache_payload: continue - entrypoint_cls.on_explorer_update( - cache_payload=cache_payload, - summary=summary, - run_id=run_id, - ) + if cache_payload.get("organization_id") != organization.id: + # run_id is globally unique in Seer, so only one entrypoint will + # have a cache entry per run. An org mismatch here is anomalous; + # return rather than continue to abort the entire method. + lifecycle.record_failure(failure_reason="org_mismatch") + return + + with SeerOperatorEventLifecycleMetric( + interaction_type=SeerOperatorInteractionType.ENTRYPOINT_ON_EXPLORER_UPDATE, + entrypoint_key=str(entrypoint_key), + ).capture() as ept_lifecycle: + try: + entrypoint_cls.on_explorer_update( + cache_payload=cache_payload, + summary=summary, + run_id=run_id, + ) + except Exception as e: + ept_lifecycle.record_failure(failure_reason=e) diff --git a/src/sentry/seer/explorer/client.py b/src/sentry/seer/explorer/client.py index 62aea6c6371e95..9e2d1baf14f010 100644 --- a/src/sentry/seer/explorer/client.py +++ b/src/sentry/seer/explorer/client.py @@ -35,6 +35,7 @@ from sentry.seer.seer_setup import has_seer_access_with_detail from sentry.seer.signed_seer_api import SeerViewerContext from sentry.users.models.user import User +from sentry.users.services.user import RpcUser logger = logging.getLogger(__name__) @@ -170,7 +171,7 @@ def execute(cls, organization: Organization, run_id: int) -> None: Args: organization: Sentry organization - user: User for permission checks and user-specific context (can be User, AnonymousUser, or None) + user: User for permission checks and user-specific context (can be User, RpcUser, AnonymousUser, or None) project: Optional project for project-scoped runs (e.g. autofix for an issue) category_key: Optional category key for filtering/grouping runs (e.g., "bug-fixer", "trace-analyzer"). Must be provided together with category_value. Makes it easy to retrieve runs for your feature later. category_value: Optional category value for filtering/grouping runs (e.g., issue ID, trace ID). Must be provided together with category_key. Makes it easy to retrieve a specific run for your feature later. @@ -185,7 +186,7 @@ def execute(cls, organization: Organization, run_id: int) -> None: def __init__( self, organization: Organization, - user: User | AnonymousUser | None = None, + user: User | RpcUser | AnonymousUser | None = None, project: Project | None = None, category_key: str | None = None, category_value: str | None = None, diff --git a/src/sentry/seer/explorer/client_utils.py b/src/sentry/seer/explorer/client_utils.py index 1438e4ed10644c..cf699f2ffb3f05 100644 --- a/src/sentry/seer/explorer/client_utils.py +++ b/src/sentry/seer/explorer/client_utils.py @@ -188,7 +188,7 @@ def has_seer_explorer_access_with_detail( def collect_user_org_context( - user: SentryUser | AnonymousUser | None, + user: SentryUser | RpcUser | AnonymousUser | None, organization: Organization, request: Request | None = None, ) -> dict[str, Any]: @@ -234,7 +234,7 @@ def collect_user_org_context( # Handle name attribute - SentryUser has name user_name: str | None = None - if isinstance(user, SentryUser): + if isinstance(user, (SentryUser, RpcUser)): user_name = user.name # Get user's timezone setting (IANA timezone name, e.g., "America/Los_Angeles") diff --git a/src/sentry/snuba/referrer.py b/src/sentry/snuba/referrer.py index 660fe25378c653..43135a2180f5b3 100644 --- a/src/sentry/snuba/referrer.py +++ b/src/sentry/snuba/referrer.py @@ -575,6 +575,7 @@ class Referrer(StrEnum): API_PREPROD_TAG_VALUES_RPC = "api.preprod.tags-values.rpc" API_PROCESSING_ERRORS_TAG_KEYS_RPC = "api.processing-errors.tags-keys.rpc" API_PROCESSING_ERRORS_TAG_VALUES_RPC = "api.processing-errors.tags-values.rpc" + API_TRACE_ITEM_ATTRIBUTE_VALIDATE = "api.trace-item.attribute-validate" API_SPANS_TAG_KEYS = "api.spans.tags-keys" API_SPANS_TAG_KEYS_RPC = "api.spans.tags-keys.rpc" API_SPANS_TAG_VALUES = "api.spans.tags-values" diff --git a/static/app/components/replays/alerts/missingReplayAlert.tsx b/static/app/components/replays/alerts/missingReplayAlert.tsx index e71b4c06882ead..90ce02e7a8565f 100644 --- a/static/app/components/replays/alerts/missingReplayAlert.tsx +++ b/static/app/components/replays/alerts/missingReplayAlert.tsx @@ -7,6 +7,7 @@ import {ExternalLink, Link} from '@sentry/scraps/link'; import {List} from 'sentry/components/list'; import {ListItem} from 'sentry/components/list/listItem'; import {t, tct} from 'sentry/locale'; +import {normalizeUrl} from 'sentry/utils/url/normalizeUrl'; interface Props { orgSlug: string; @@ -18,7 +19,9 @@ export function MissingReplayAlert({orgSlug}: Props) { tct( 'The replay was rate-limited and could not be accepted. [link:View the stats page] for more information.', { - link: , + link: ( + + ), } ), t('The replay has been deleted by a member in your organization.'), diff --git a/static/app/gettingStartedDocs/dotnet/metrics.spec.tsx b/static/app/gettingStartedDocs/dotnet/metrics.spec.tsx index 36780c89c56db6..f0d4e0e0c716ed 100644 --- a/static/app/gettingStartedDocs/dotnet/metrics.spec.tsx +++ b/static/app/gettingStartedDocs/dotnet/metrics.spec.tsx @@ -13,9 +13,7 @@ describe('metrics', () => { }); expect( - screen.getByText( - textWithMarkupMatcher(/SentrySdk\.Experimental\.Metrics\.EmitCounter/) - ) + screen.getByText(textWithMarkupMatcher(/SentrySdk\.Metrics\.EmitCounter/)) ).toBeInTheDocument(); }); @@ -25,9 +23,7 @@ describe('metrics', () => { }); expect( - screen.queryByText( - textWithMarkupMatcher(/SentrySdk\.Experimental\.Metrics\.EmitCounter/) - ) + screen.queryByText(textWithMarkupMatcher(/SentrySdk\.Metrics\.EmitCounter/)) ).not.toBeInTheDocument(); }); }); diff --git a/static/app/gettingStartedDocs/dotnet/metrics.tsx b/static/app/gettingStartedDocs/dotnet/metrics.tsx index a2a2d34e14a36b..9cf38682c11d24 100644 --- a/static/app/gettingStartedDocs/dotnet/metrics.tsx +++ b/static/app/gettingStartedDocs/dotnet/metrics.tsx @@ -27,11 +27,11 @@ export const metricsVerify = (params: DocsParams): ContentBlock => ({ language: 'csharp', code: `using Sentry; -SentrySdk.Experimental.Metrics.EmitCounter("button_click", 5, +SentrySdk.Metrics.EmitCounter("button_click", 5, [new KeyValuePair("browser", "Firefox"), new KeyValuePair("app_version", "1.0.0")]); -SentrySdk.Experimental.Metrics.EmitDistribution("page_load", 15.0, MeasurementUnit.Duration.Millisecond, +SentrySdk.Metrics.EmitDistribution("page_load", 15.0, MeasurementUnit.Duration.Millisecond, [new KeyValuePair("page", "/home")]); -SentrySdk.Experimental.Metrics.EmitGauge("page_load", 15.0, MeasurementUnit.Duration.Millisecond, +SentrySdk.Metrics.EmitGauge("page_load", 15.0, MeasurementUnit.Duration.Millisecond, [new KeyValuePair("page", "/home")]); `, }, @@ -52,7 +52,7 @@ export const metrics: OnboardingConfig = { { type: 'text', text: tct( - 'Install our .NET SDK with a minimum version that supports metrics ([code:6.1.0] or higher).', + 'Install our .NET SDK with a minimum version that supports metrics ([code:6.3.0] or higher).', { code: , } @@ -84,7 +84,7 @@ export const metrics: OnboardingConfig = { { type: 'text', text: tct( - 'Metrics are automatically enabled in your [code:SentrySdk.Init] configuration. You can emit metrics using the [code:SentrySdk.Experimental.Metrics] API.', + 'Metrics are automatically enabled in your [code:SentrySdk.Init] configuration. You can emit metrics using the [code:SentrySdk.Metrics] API.', { code: , } @@ -100,11 +100,11 @@ SentrySdk.Init(options => options.Dsn = "${params.dsn.public}"; }); -SentrySdk.Experimental.Metrics.EmitCounter("button_click", 5, +SentrySdk.Metrics.EmitCounter("button_click", 5, [new KeyValuePair("browser", "Firefox"), new KeyValuePair("app_version", "1.0.0")]); -SentrySdk.Experimental.Metrics.EmitDistribution("page_load", 15.0, MeasurementUnit.Duration.Millisecond, +SentrySdk.Metrics.EmitDistribution("page_load", 15.0, MeasurementUnit.Duration.Millisecond, [new KeyValuePair("page", "/home")]); -SentrySdk.Experimental.Metrics.EmitGauge("page_load", 15.0, MeasurementUnit.Duration.Millisecond, +SentrySdk.Metrics.EmitGauge("page_load", 15.0, MeasurementUnit.Duration.Millisecond, [new KeyValuePair("page", "/home")]);`, }, { diff --git a/static/app/gettingStartedDocs/dotnet/utils.tsx b/static/app/gettingStartedDocs/dotnet/utils.tsx index a06806b8382782..91e4aaff1e45a6 100644 --- a/static/app/gettingStartedDocs/dotnet/utils.tsx +++ b/static/app/gettingStartedDocs/dotnet/utils.tsx @@ -1,10 +1,10 @@ import type {DocsParams} from 'sentry/components/onboarding/gettingStartedDoc/types'; import {getPackageVersion} from 'sentry/utils/gettingStartedDocs/getPackageVersion'; -// Sentry SDK for .NET 6.1.0 adds initial experimental support for Metrics +// Sentry SDK for .NET 6.3.0 adds stable APIs for Metrics export const getInstallSnippetPackageManager = (params: DocsParams) => ` -Install-Package Sentry -Version ${getPackageVersion(params, 'sentry.dotnet', '6.1.0')}`; +Install-Package Sentry -Version ${getPackageVersion(params, 'sentry.dotnet', '6.3.0')}`; -// Sentry SDK for .NET 6.1.0 adds initial experimental support for Metrics +// Sentry SDK for .NET 6.3.0 adds stable APIs for Metrics export const getInstallSnippetCoreCli = (params: DocsParams) => ` -dotnet add package Sentry -v ${getPackageVersion(params, 'sentry.dotnet', '6.1.0')}`; +dotnet add package Sentry -v ${getPackageVersion(params, 'sentry.dotnet', '6.3.0')}`; diff --git a/static/app/views/explore/metrics/metricToolbar/metricSelector.tsx b/static/app/views/explore/metrics/metricToolbar/metricSelector.tsx index 6c99ae4f9819ad..4283506deda91e 100644 --- a/static/app/views/explore/metrics/metricToolbar/metricSelector.tsx +++ b/static/app/views/explore/metrics/metricToolbar/metricSelector.tsx @@ -355,17 +355,6 @@ export function MetricSelector({ overscan: 20, }); - const focusedIndex = useMemo( - () => collectionItems.findIndex(item => item.key === focusedKey), - [collectionItems, focusedKey] - ); - - useEffect(() => { - if (focusedIndex >= 0 && isOpen) { - virtualizer.scrollToIndex(focusedIndex, {align: 'auto'}); - } - }, [focusedIndex, isOpen, virtualizer]); - const highlightedOption = focusedKey ? (displayedOptionsMap.get(String(focusedKey)) ?? null) : null; diff --git a/static/app/views/performance/transactionSummary/transactionProfiles/content.tsx b/static/app/views/performance/transactionSummary/transactionProfiles/content.tsx index 1ca940e6007160..6c8a759768573f 100644 --- a/static/app/views/performance/transactionSummary/transactionProfiles/content.tsx +++ b/static/app/views/performance/transactionSummary/transactionProfiles/content.tsx @@ -95,6 +95,7 @@ export function TransactionProfilesContent(props: TransactionProfilesContentProp const {data, status} = useAggregateFlamegraphQuery({ query, + ...(isEAP ? {dataSource: 'spans' as const} : {}), }); const [frameFilter, setFrameFilter] = useLocalStorageState< diff --git a/static/app/views/performance/transactionSummary/transactionProfiles/index.tsx b/static/app/views/performance/transactionSummary/transactionProfiles/index.tsx index 297f2a78cb9226..7d83a8447839a2 100644 --- a/static/app/views/performance/transactionSummary/transactionProfiles/index.tsx +++ b/static/app/views/performance/transactionSummary/transactionProfiles/index.tsx @@ -5,6 +5,7 @@ import * as Layout from 'sentry/components/layouts/thirds'; import {DatePageFilter} from 'sentry/components/pageFilters/date/datePageFilter'; import {EnvironmentPageFilter} from 'sentry/components/pageFilters/environment/environmentPageFilter'; import {PageFilterBar} from 'sentry/components/pageFilters/pageFilterBar'; +import {useSpanSearchQueryBuilderProps} from 'sentry/components/performance/spanSearchQueryBuilder'; import {TransactionSearchQueryBuilder} from 'sentry/components/performance/transactionSearchQueryBuilder'; import {DataCategory} from 'sentry/types/core'; import {isAggregateField} from 'sentry/utils/discover/fields'; @@ -16,10 +17,33 @@ import {useMaxPickableDays} from 'sentry/utils/useMaxPickableDays'; import {useNavigate} from 'sentry/utils/useNavigate'; import {useOrganization} from 'sentry/utils/useOrganization'; import {useProjects} from 'sentry/utils/useProjects'; +import {TraceItemSearchQueryBuilder} from 'sentry/views/explore/components/traceItemSearchQueryBuilder'; +import {useTransactionSummaryEAP} from 'sentry/views/performance/eap/useTransactionSummaryEAP'; import {redirectToPerformanceHomepage} from 'sentry/views/performance/transactionSummary/pageLayout'; import {TransactionProfilesContent} from './content'; +function EAPSearchBar({ + projects, + initialQuery, + onSearch, +}: { + initialQuery: string; + onSearch: (query: string) => void; + projects: number[]; +}) { + const {spanSearchQueryBuilderProps} = useSpanSearchQueryBuilderProps({ + projects, + initialQuery, + onSearch, + searchSource: 'transaction_profiles', + }); + + return ( + + ); +} + interface ProfilesProps { transaction: string; } @@ -28,6 +52,7 @@ function Profiles({transaction}: ProfilesProps) { const navigate = useNavigate(); const location = useLocation(); const {projects} = useProjects(); + const shouldUseEAP = useTransactionSummaryEAP(); const project = projects.find(p => p.id === location.query.project); @@ -38,7 +63,11 @@ function Profiles({transaction}: ProfilesProps) { const query = useMemo(() => { const conditions = new MutableSearch(rawQuery); - conditions.setFilterValues('event.type', ['transaction']); + if (shouldUseEAP) { + conditions.setFilterValues('is_transaction', ['true']); + } else { + conditions.setFilterValues('event.type', ['transaction']); + } conditions.setFilterValues('transaction', [transaction]); Object.keys(conditions.filters).forEach(field => { @@ -48,7 +77,7 @@ function Profiles({transaction}: ProfilesProps) { }); return conditions.formatString(); - }, [transaction, rawQuery]); + }, [transaction, rawQuery, shouldUseEAP]); const handleSearch = useCallback( (searchQuery: string) => { @@ -81,12 +110,20 @@ function Profiles({transaction}: ProfilesProps) { - + {shouldUseEAP ? ( + + ) : ( + + )} diff --git a/static/app/views/settings/account/apiApplications/details.spec.tsx b/static/app/views/settings/account/apiApplications/details.spec.tsx index 083367bcf618b0..a1f788316ed308 100644 --- a/static/app/views/settings/account/apiApplications/details.spec.tsx +++ b/static/app/views/settings/account/apiApplications/details.spec.tsx @@ -8,6 +8,8 @@ import { import ApiApplicationDetails from 'sentry/views/settings/account/apiApplications/details'; describe('ApiApplicationDetails', () => { + const oauthBaseUrl = 'https://sentry-jest-tests.example.com/oauth'; + it('renders basic details for confidential client', async () => { MockApiClient.addMockResponse({ url: '/api-applications/abcd/', @@ -55,6 +57,10 @@ describe('ApiApplicationDetails', () => { expect(screen.getByLabelText('Client Secret')).toBeInTheDocument(); expect(screen.getByLabelText('Authorization URL')).toBeInTheDocument(); expect(screen.getByLabelText('Token URL')).toBeInTheDocument(); + expect(screen.getByDisplayValue(`${oauthBaseUrl}/authorize/`)).toBeInTheDocument(); + expect(screen.getByDisplayValue(`${oauthBaseUrl}/token/`)).toBeInTheDocument(); + expect(screen.queryByLabelText('Device Authorization URL')).not.toBeInTheDocument(); + expect(screen.queryByLabelText('Device Verification URL')).not.toBeInTheDocument(); }); it('handles client secret rotation', async () => { @@ -160,6 +166,10 @@ describe('ApiApplicationDetails', () => { expect(screen.getByLabelText('Client ID')).toBeInTheDocument(); expect(screen.getByDisplayValue('public-app')).toBeInTheDocument(); expect(screen.getByDisplayValue('Public CLI App')).toBeInTheDocument(); + expect(screen.getByDisplayValue(`${oauthBaseUrl}/authorize/`)).toBeInTheDocument(); + expect(screen.getByDisplayValue(`${oauthBaseUrl}/token/`)).toBeInTheDocument(); + expect(screen.getByDisplayValue(`${oauthBaseUrl}/device/code/`)).toBeInTheDocument(); + expect(screen.getByDisplayValue(`${oauthBaseUrl}/device/`)).toBeInTheDocument(); }); it('renders confidential client with client secret section', async () => { @@ -202,5 +212,7 @@ describe('ApiApplicationDetails', () => { expect( screen.queryByText(/This is a public client, designed for CLIs/) ).not.toBeInTheDocument(); + expect(screen.queryByLabelText('Device Authorization URL')).not.toBeInTheDocument(); + expect(screen.queryByLabelText('Device Verification URL')).not.toBeInTheDocument(); }); }); diff --git a/static/app/views/settings/account/apiApplications/details.tsx b/static/app/views/settings/account/apiApplications/details.tsx index 9369aea622e65b..aac78bc69d44db 100644 --- a/static/app/views/settings/account/apiApplications/details.tsx +++ b/static/app/views/settings/account/apiApplications/details.tsx @@ -1,5 +1,6 @@ import {Fragment} from 'react'; import styled from '@emotion/styled'; +import trimEnd from 'lodash/trimEnd'; import {Alert} from '@sentry/scraps/alert'; import {Tag} from '@sentry/scraps/badge'; @@ -54,6 +55,7 @@ function ApiApplicationsDetails() { const queryClient = useQueryClient(); const urlPrefix = ConfigStore.get('urlPrefix'); + const oauthBaseUrl = `${trimEnd(urlPrefix, '/')}/oauth`; const { data: app, @@ -173,12 +175,27 @@ function ApiApplicationsDetails() { )} - {`${urlPrefix}/oauth/authorize/`} + {`${oauthBaseUrl}/authorize/`} - {`${urlPrefix}/oauth/token/`} + {`${oauthBaseUrl}/token/`} + + {app.isPublic && ( + + + {`${oauthBaseUrl}/device/code/`} + + + + {`${oauthBaseUrl}/device/`} + + + )} diff --git a/static/app/views/settings/dynamicSampling/organizationSampling.tsx b/static/app/views/settings/dynamicSampling/organizationSampling.tsx index 5ad9d9c05b6143..162ce9a206f22c 100644 --- a/static/app/views/settings/dynamicSampling/organizationSampling.tsx +++ b/static/app/views/settings/dynamicSampling/organizationSampling.tsx @@ -26,18 +26,20 @@ const UNSAVED_CHANGES_MESSAGE = t( 'You have unsaved changes, are you sure you want to leave?' ); +export const sampleRateField = z + .string() + .min(1, t('Please enter a valid number')) + .refine(val => !isNaN(Number(val)), {message: t('Please enter a valid number')}) + .refine( + val => { + const n = Number(val); + return n >= 0 && n <= 100; + }, + {message: t('Must be between 0% and 100%')} + ); + export const targetSampleRateSchema = z.object({ - targetSampleRate: z - .string() - .min(1, t('Please enter a valid number')) - .refine(val => !isNaN(Number(val)), {message: t('Please enter a valid number')}) - .refine( - val => { - const n = Number(val); - return n >= 0 && n <= 100; - }, - {message: t('Must be between 0% and 100%')} - ), + targetSampleRate: sampleRateField, }); export function OrganizationSampling() { @@ -80,8 +82,8 @@ export function OrganizationSampling() { return ( - ({isDirty: s.isDirty, canSubmit: s.canSubmit})}> - {({isDirty, canSubmit}) => ( + ({isDirty: s.isDirty})}> + {({isDirty}) => ( - + {t('Apply Changes')} diff --git a/static/app/views/settings/dynamicSampling/projectSampling.spec.tsx b/static/app/views/settings/dynamicSampling/projectSampling.spec.tsx new file mode 100644 index 00000000000000..a4cb90fab07e88 --- /dev/null +++ b/static/app/views/settings/dynamicSampling/projectSampling.spec.tsx @@ -0,0 +1,222 @@ +import {OrganizationFixture} from 'sentry-fixture/organization'; +import {ProjectFixture} from 'sentry-fixture/project'; + +import {act, render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; + +import {ProjectsStore} from 'sentry/stores/projectsStore'; + +import {ProjectSampling} from './projectSampling'; + +jest.mock('@tanstack/react-virtual', () => ({ + useVirtualizer: jest.fn(({count}: {count: number}) => ({ + getVirtualItems: jest.fn(() => + Array.from({length: count}, (_, index) => ({ + key: index, + index, + start: index * 63, + size: 63, + })) + ), + getTotalSize: jest.fn(() => count * 63), + measure: jest.fn(), + })), +})); + +describe('ProjectSampling', () => { + const project = ProjectFixture({id: '1', slug: 'project-slug'}); + const organization = OrganizationFixture({ + slug: 'org-slug', + access: ['org:write'], + samplingMode: 'project', + }); + + beforeEach(() => { + MockApiClient.clearMockResponses(); + act(() => ProjectsStore.loadInitialData([project])); + + MockApiClient.addMockResponse({ + url: '/organizations/org-slug/sampling/project-root-counts/', + body: { + data: [ + [ + { + by: {project: 'project-slug', target_project_id: '1'}, + totals: 1000, + series: [], + }, + ], + ], + end: '', + intervals: [], + start: '', + }, + }); + + MockApiClient.addMockResponse({ + url: '/organizations/org-slug/sampling/project-rates/', + body: [{id: 1, sampleRate: 0.5}], + }); + }); + + async function waitForProjectRateInput() { + return screen.findByRole('spinbutton', { + name: 'Sample rate for project-slug', + }); + } + + it('renders project rate inputs with initial values', async () => { + // The input briefly transitions from uncontrolled to controlled as form + // state initializes with the fetched project rates. + jest.spyOn(console, 'error').mockImplementation(); + + render(, {organization}); + + const input = await waitForProjectRateInput(); + expect(input).toHaveValue(50); + }); + + it('enables Reset button after changing a project rate', async () => { + render(, {organization}); + + const input = await waitForProjectRateInput(); + expect(screen.getByRole('button', {name: 'Reset'})).toBeDisabled(); + + await userEvent.clear(input); + await userEvent.type(input, '30'); + + expect(screen.getByRole('button', {name: 'Reset'})).toBeEnabled(); + }); + + it('resets the input back to the saved value when Reset is clicked', async () => { + render(, {organization}); + + const input = await waitForProjectRateInput(); + await userEvent.clear(input); + await userEvent.type(input, '30'); + + await userEvent.click(screen.getByRole('button', {name: 'Reset'})); + + expect(input).toHaveValue(50); + }); + + it('shows validation error for empty value on submit', async () => { + render(, {organization}); + + const input = await waitForProjectRateInput(); + await userEvent.clear(input); + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + expect(await screen.findByText('Please enter a valid number')).toBeInTheDocument(); + }); + + it('calls the API with the correct payload on save', async () => { + const putMock = MockApiClient.addMockResponse({ + url: '/organizations/org-slug/sampling/project-rates/', + method: 'PUT', + body: [{id: 1, sampleRate: 0.3}], + }); + + render(, {organization}); + + const input = await waitForProjectRateInput(); + await userEvent.clear(input); + await userEvent.type(input, '30'); + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + await waitFor(() => { + expect(putMock).toHaveBeenCalledWith( + '/organizations/org-slug/sampling/project-rates/', + expect.objectContaining({data: [{id: 1, sampleRate: 0.3}]}) + ); + }); + }); + + it('resets form to clean state after a successful save', async () => { + MockApiClient.addMockResponse({ + url: '/organizations/org-slug/sampling/project-rates/', + method: 'PUT', + body: [{id: 1, sampleRate: 0.3}], + }); + + render(, {organization}); + + const input = await waitForProjectRateInput(); + await userEvent.clear(input); + await userEvent.type(input, '30'); + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + await waitFor(() => + expect(screen.getByRole('button', {name: 'Reset'})).toBeDisabled() + ); + }); + + it('keeps form dirty after an API error', async () => { + MockApiClient.addMockResponse({ + url: '/organizations/org-slug/sampling/project-rates/', + method: 'PUT', + statusCode: 500, + body: {detail: 'Internal Server Error'}, + }); + + render(, {organization}); + + const input = await waitForProjectRateInput(); + await userEvent.clear(input); + await userEvent.type(input, '30'); + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + await waitFor(() => + expect(screen.getByRole('button', {name: 'Reset'})).toBeEnabled() + ); + }); + + it('updates project rates atomically via bulk org rate edit', async () => { + const putMock = MockApiClient.addMockResponse({ + url: '/organizations/org-slug/sampling/project-rates/', + method: 'PUT', + body: [{id: 1, sampleRate: 0.8}], + }); + + render(, {organization}); + + await waitForProjectRateInput(); + + // Activate bulk edit mode + await userEvent.click( + screen.getByRole('button', {name: 'Proportionally scale project rates'}) + ); + + // Type a new org rate — this should update all project rates in one atomic call + const orgRateInput = screen.getAllByRole('spinbutton')[0]!; + await userEvent.clear(orgRateInput); + await userEvent.type(orgRateInput, '80'); + + // The project rate should have been scaled + const projectInput = screen.getByRole('spinbutton', { + name: 'Sample rate for project-slug', + }); + expect(projectInput).toHaveValue(80); + + // Submit and verify the API call + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + await waitFor(() => { + expect(putMock).toHaveBeenCalledWith( + '/organizations/org-slug/sampling/project-rates/', + expect.objectContaining({data: [{id: 1, sampleRate: 0.8}]}) + ); + }); + }); + + it('disables Apply Changes for users without org:write access', async () => { + const orgWithoutAccess = OrganizationFixture({ + access: [], + samplingMode: 'project', + }); + + render(, {organization: orgWithoutAccess}); + + await waitForProjectRateInput(); + expect(screen.getByRole('button', {name: 'Apply Changes'})).toBeDisabled(); + }); +}); diff --git a/static/app/views/settings/dynamicSampling/projectSampling.tsx b/static/app/views/settings/dynamicSampling/projectSampling.tsx index 43f868a83f6350..e360db91c6f413 100644 --- a/static/app/views/settings/dynamicSampling/projectSampling.tsx +++ b/static/app/views/settings/dynamicSampling/projectSampling.tsx @@ -1,7 +1,9 @@ -import {Fragment, useMemo, useState} from 'react'; +import {Fragment, useCallback, useEffect, useMemo, useState} from 'react'; import styled from '@emotion/styled'; +import {z} from 'zod'; import {Button} from '@sentry/scraps/button'; +import {defaultFormOptions, useScrapsForm} from '@sentry/scraps/form'; import {Flex} from '@sentry/scraps/layout'; import { @@ -12,13 +14,13 @@ import { import {LoadingError} from 'sentry/components/loadingError'; import {t} from 'sentry/locale'; import {OnRouteLeave} from 'sentry/utils/reactRouter6Compat/onRouteLeave'; +import {sampleRateField} from 'sentry/views/settings/dynamicSampling/organizationSampling'; import {ProjectionPeriodControl} from 'sentry/views/settings/dynamicSampling/projectionPeriodControl'; import {ProjectsEditTable} from 'sentry/views/settings/dynamicSampling/projectsEditTable'; import {SamplingModeSwitch} from 'sentry/views/settings/dynamicSampling/samplingModeSwitch'; import {mapArrayToObject} from 'sentry/views/settings/dynamicSampling/utils'; import {useHasDynamicSamplingWriteAccess} from 'sentry/views/settings/dynamicSampling/utils/access'; import {parsePercent} from 'sentry/views/settings/dynamicSampling/utils/parsePercent'; -import {projectSamplingForm} from 'sentry/views/settings/dynamicSampling/utils/projectSamplingForm'; import { useProjectSampleCounts, type ProjectionSamplePeriod, @@ -28,11 +30,14 @@ import { useUpdateSamplingProjectRates, } from 'sentry/views/settings/dynamicSampling/utils/useSamplingProjectRates'; -const {useFormState, FormProvider} = projectSamplingForm; const UNSAVED_CHANGES_MESSAGE = t( 'You have unsaved changes, are you sure you want to leave?' ); +const projectSamplingSchema = z.object({ + projectRates: z.record(z.string(), sampleRateField), +}); + export function ProjectSampling() { const hasAccess = useHasDynamicSamplingWriteAccess(); const [period, setPeriod] = useState('24h'); @@ -54,37 +59,55 @@ export function ProjectSampling() { [sampleRatesQuery.data] ); - const initialValues = useMemo(() => ({projectRates}), [projectRates]); - - const formState = useFormState({ - initialValues, - enableReInitialize: true, - }); - - const handleReset = () => { - formState.reset(); - setEditMode('single'); - }; - - const handleSubmit = () => { - const ratesArray = Object.entries(formState.fields.projectRates.value).map( - ([id, rate]) => ({ + const [savedProjectRates, setSavedProjectRates] = + useState>(projectRates); + + const form = useScrapsForm({ + ...defaultFormOptions, + defaultValues: { + projectRates, + }, + validators: { + onDynamic: projectSamplingSchema, + }, + onSubmit: async ({value, formApi}) => { + const ratesArray = Object.entries(value.projectRates).map(([id, rate]) => ({ id: Number(id), sampleRate: parsePercent(rate), - }) - ); - addLoadingMessage(t('Saving changes...')); - updateSamplingProjectRates.mutate(ratesArray, { - onSuccess: () => { - formState.save(); + })); + addLoadingMessage(t('Saving changes...')); + try { + await updateSamplingProjectRates.mutateAsync(ratesArray); + setSavedProjectRates(value.projectRates); setEditMode('single'); + formApi.reset(value); addSuccessMessage(t('Changes applied')); - }, - onError: () => { + } catch { addErrorMessage(t('Unable to save changes. Please try again.')); - }, - }); - }; + } + }, + }); + + const handleProjectRateChange = useCallback( + (projectId: string, rate: string) => { + form.setFieldValue(`projectRates.${projectId}`, rate); + }, + [form] + ); + + const handleBulkProjectRateChange = useCallback( + (updates: Record) => { + form.setFieldValue('projectRates', prev => ({...prev, ...updates})); + }, + [form] + ); + + // Mirror enableReInitialize: reset the form whenever the server data changes + useEffect(() => { + form.reset({projectRates}); + setSavedProjectRates(projectRates); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [projectRates]); const initialTargetRate = useMemo(() => { const sampleRates = sampleRatesQuery.data ?? []; @@ -105,52 +128,75 @@ export function ProjectSampling() { ); }, [sampleRatesQuery.data, sampleCountsQuery.data]); - const isFormActionDisabled = - !hasAccess || - sampleRatesQuery.isPending || - updateSamplingProjectRates.isPending || - !formState.hasChanged; - return ( - - - locationChange.currentLocation.pathname !== - locationChange.nextLocation.pathname && formState.hasChanged - } - /> - - - - - {sampleCountsQuery.isError ? ( - - ) : ( - + ({ + isDirty: s.isDirty, + currentProjectRates: s.values.projectRates, + fieldMeta: s.fieldMeta, + })} + > + {({isDirty, currentProjectRates, fieldMeta}) => { + const projectErrors: Record = {}; + for (const id of Object.keys(currentProjectRates)) { + const error = fieldMeta[`projectRates.${id}`]?.errors?.[0]?.message; + if (error) { + projectErrors[id] = error; + } + } + + return ( - - + + locationChange.currentLocation.pathname !== + locationChange.nextLocation.pathname && isDirty + } + /> + + + + + {sampleCountsQuery.isError ? ( + + ) : ( + + + + {t('Apply Changes')} + + + } + /> + )} + - } - /> - )} - - + ); + }} + + ); } diff --git a/static/app/views/settings/dynamicSampling/projectsEditTable.tsx b/static/app/views/settings/dynamicSampling/projectsEditTable.tsx index d52da4a1ca2106..2812017093cb1b 100644 --- a/static/app/views/settings/dynamicSampling/projectsEditTable.tsx +++ b/static/app/views/settings/dynamicSampling/projectsEditTable.tsx @@ -13,7 +13,6 @@ import {SamplingBreakdown} from 'sentry/views/settings/dynamicSampling/samplingB import {mapArrayToObject} from 'sentry/views/settings/dynamicSampling/utils'; import {formatPercent} from 'sentry/views/settings/dynamicSampling/utils/formatPercent'; import {parsePercent} from 'sentry/views/settings/dynamicSampling/utils/parsePercent'; -import {projectSamplingForm} from 'sentry/views/settings/dynamicSampling/utils/projectSamplingForm'; import {scaleSampleRates} from 'sentry/views/settings/dynamicSampling/utils/scaleSampleRates'; import type { ProjectionSamplePeriod, @@ -24,13 +23,17 @@ interface Props { actions: React.ReactNode; editMode: 'single' | 'bulk'; isLoading: boolean; + onBulkProjectRateChange: (updates: Record) => void; onEditModeChange: (mode: 'single' | 'bulk') => void; + onProjectRateChange: (projectId: string, rate: string) => void; period: ProjectionSamplePeriod; + projectErrors: Record; + projectRates: Record; sampleCounts: ProjectSampleCount[]; + savedProjectRates: Record; } -const {useFormField} = projectSamplingForm; -const EMPTY_ARRAY: any = []; +const EMPTY_ARRAY: never[] = []; export function ProjectsEditTable({ actions, @@ -39,9 +42,13 @@ export function ProjectsEditTable({ editMode, period, onEditModeChange, + onBulkProjectRateChange, + onProjectRateChange, + projectRates, + projectErrors, + savedProjectRates, }: Props) { const {projects, fetching} = useProjects(); - const {value, initialValue, error, onChange} = useFormField('projectRates'); const [isBulkEditEnabled, setIsBulkEditEnabled] = useState(false); const [orgRate, setOrgRate] = useState(''); @@ -59,22 +66,19 @@ export function ProjectsEditTable({ const handleProjectChange = useCallback( (projectId: string, newRate: string) => { - onChange(prev => ({ - ...prev, - [projectId]: newRate, - })); + onProjectRateChange(projectId, newRate); onEditModeChange('single'); }, - [onChange, onEditModeChange] + [onProjectRateChange, onEditModeChange] ); const handleOrgChange = useCallback( (newRate: string) => { - // Editing the org rate will transition the logic to bulk edit mode + // Editing the org rate will transition the logic to bulk edit mode. // On the first edit, we need to snapshot the current project rates as scaling baseline - // to avoid rounding errors when scaling the sample rates up and down + // to avoid rounding errors when scaling the sample rates up and down. if (editMode === 'single') { - projectRateSnapshotRef.current = value; + projectRateSnapshotRef.current = projectRates; } const cappedOrgRate = parsePercent(newRate, 1); @@ -84,7 +88,7 @@ export function ProjectsEditTable({ sampleRate: rate ? parsePercent(rate) : 0, count: dataByProjectId[projectId]?.count ?? 0, })) - // We do not wan't to bulk edit inactive projects as they have no effect on the outcome + // We do not want to bulk edit inactive projects as they have no effect on the outcome .filter(item => item.count !== 0); const {scaledItems} = scaleSampleRates({ @@ -98,15 +102,11 @@ export function ProjectsEditTable({ valueSelector: item => formatPercent(item.sampleRate), }); - // Update the form state (project values) with the new sample rates - onChange(prev => { - return {...prev, ...newProjectValues}; - }); - + onBulkProjectRateChange(newProjectValues); setOrgRate(newRate); onEditModeChange('bulk'); }, - [dataByProjectId, editMode, onChange, onEditModeChange, value] + [dataByProjectId, editMode, onBulkProjectRateChange, onEditModeChange, projectRates] ); const handleBulkEditChange = useCallback((newIsActive: boolean) => { @@ -129,12 +129,12 @@ export function ProjectsEditTable({ ownCount: item?.ownCount || 0, subProjects: item?.subProjects ?? EMPTY_ARRAY, project, - initialSampleRate: initialValue[project.id]!, - sampleRate: value[project.id]!, - error: error?.[project.id], + initialSampleRate: savedProjectRates[project.id]!, + sampleRate: projectRates[project.id]!, + error: projectErrors[project.id], }; }), - [dataByProjectId, error, initialValue, projects, value] + [dataByProjectId, projectErrors, savedProjectRates, projects, projectRates] ); const totalSpanCount = useMemo( @@ -142,35 +142,42 @@ export function ProjectsEditTable({ [items] ); - // In bulk edit mode, we display the org rate from the input state - // In single edit mode, we display the estimated org rate based on the current sample rates + // In bulk edit mode, we display the org rate from the input state. + // In single edit mode, we display the estimated org rate based on the current sample rates. const displayedOrgRate = useMemo(() => { if (editMode === 'bulk') { return orgRate; } const totalSampledSpans = items.reduce( - (acc, item) => acc + item.count * parsePercent(value[item.project.id], 1), + (acc, item) => acc + item.count * parsePercent(projectRates[item.project.id], 1), 0 ); + if (totalSpanCount === 0) { + return formatPercent(0); + } return formatPercent(totalSampledSpans / totalSpanCount); - }, [editMode, items, orgRate, totalSpanCount, value]); + }, [editMode, items, orgRate, totalSpanCount, projectRates]); const initialOrgRate = useMemo(() => { const totalSampledSpans = items.reduce( - (acc, item) => acc + item.count * parsePercent(initialValue[item.project.id], 1), + (acc, item) => + acc + item.count * parsePercent(savedProjectRates[item.project.id], 1), 0 ); + if (totalSpanCount === 0) { + return formatPercent(0); + } return formatPercent(totalSampledSpans / totalSpanCount); - }, [initialValue, items, totalSpanCount]); + }, [savedProjectRates, items, totalSpanCount]); const breakdownSampleRates = useMemo( () => mapArrayToObject({ - array: Object.entries(value), + array: Object.entries(projectRates), keySelector: ([projectId, _]) => projectId, valueSelector: ([_, rate]) => parsePercent(rate), }), - [value] + [projectRates] ); const isLoading = fetching || isLoadingProp; diff --git a/static/app/views/settings/dynamicSampling/projectsTable.tsx b/static/app/views/settings/dynamicSampling/projectsTable.tsx index 511fe9c37a8a97..b1af658e978f82 100644 --- a/static/app/views/settings/dynamicSampling/projectsTable.tsx +++ b/static/app/views/settings/dynamicSampling/projectsTable.tsx @@ -385,7 +385,7 @@ const TableRow = memo(function TableRow({ )} - + diff --git a/static/app/views/settings/dynamicSampling/utils/formContext.tsx b/static/app/views/settings/dynamicSampling/utils/formContext.tsx deleted file mode 100644 index 4f6010e7b107ab..00000000000000 --- a/static/app/views/settings/dynamicSampling/utils/formContext.tsx +++ /dev/null @@ -1,235 +0,0 @@ -import { - createContext, - useCallback, - useContext, - useEffect, - useMemo, - useState, -} from 'react'; -import isEqual from 'lodash/isEqual'; - -interface FormState< - FormFields extends PlainValue, - FieldErrors extends Record, -> { - /** - * State for each field in the form. - */ - fields: { - [K in keyof FormFields]: { - hasChanged: boolean; - initialValue: FormFields[K]; - onChange: (value: React.SetStateAction) => void; - value: FormFields[K]; - error?: FieldErrors[K]; - }; - }; - /** - * Whether the form has changed from the initial values. - */ - hasChanged: boolean; - /** - * Whether the form is valid. - * A form is valid if all fields pass validation. - */ - isValid: boolean; - /** - * Resets the form state to the initial values. - */ - reset: () => void; - /** - * Saves the form state by setting the initial values to the current values. - */ - save: () => void; -} - -type PlainValue = AtomicValue | PlainArray | PlainObject; -interface PlainObject { - [key: string]: PlainValue; -} -interface PlainArray extends Array {} -type AtomicValue = string | number | boolean | null | undefined; - -type FormValidators< - FormFields extends Record, - FieldErrors extends Record, -> = { - [K in keyof FormFields]?: (value: FormFields[K]) => FieldErrors[K] | undefined; -}; - -type InitialValues> = { - [K in keyof FormFields]: FormFields[K]; -}; - -type FormStateConfig< - FormFields extends Record, - FieldErrors extends Record, -> = { - /** - * The initial values for the form fields. - */ - initialValues: InitialValues; - /** - * Whether to re-initialize the form state when the initial values change. - */ - enableReInitialize?: boolean; - /** - * Validator functions for the form fields. - */ - validators?: FormValidators; -}; - -/** - * Creates a form state object with fields and validation for a given set of form fields. - */ -const useFormState = < - FormFields extends Record, - FieldErrors extends Record, ->( - config: FormStateConfig -): FormState => { - const [initialValues, setInitialValues] = useState(config.initialValues); - const [validators] = useState(config.validators); - const [values, setValues] = useState(initialValues); - const [errors, setErrors] = useState<{[K in keyof FormFields]?: FieldErrors[K]}>({}); - - useEffect(() => { - if (config.enableReInitialize) { - // eslint-disable-next-line react-you-might-not-need-an-effect/no-derived-state - setInitialValues(config.initialValues); - // eslint-disable-next-line react-you-might-not-need-an-effect/no-derived-state - setValues(config.initialValues); - setErrors({}); - } - }, [config.enableReInitialize, config.initialValues]); - - const setValue = useCallback( - (name: K, value: React.SetStateAction) => { - setValues(old => ({ - ...old, - [name]: typeof value === 'function' ? value(old[name]) : value, - })); - }, - [] - ); - - const setError = useCallback( - (name: K, error: string | undefined) => { - setErrors(old => ({...old, [name]: error})); - }, - [] - ); - - /** - * Validates a field by running the field's validator function. - */ - const validateField = useCallback( - (name: K, value: FormFields[K]) => { - const validator = validators?.[name]; - return validator?.(value); - }, - [validators] - ); - - const handleFieldChange = useCallback( - (name: K, value: React.SetStateAction) => { - setValue(name, old => { - const newValue = typeof value === 'function' ? value(old) : value; - const error = validateField(name, newValue); - setError(name, error); - return newValue; - }); - }, - [setError, setValue, validateField] - ); - - const changeHandlers = useMemo(() => { - const result: { - [K in keyof FormFields]: (value: React.SetStateAction) => void; - } = {} as any; - - for (const name in initialValues) { - result[name] = (value: React.SetStateAction) => - handleFieldChange(name, value); - } - - return result; - }, [handleFieldChange, initialValues]); - - const fields = useMemo(() => { - const result: FormState['fields'] = {} as any; - - for (const name in initialValues) { - result[name] = { - value: values[name], - onChange: changeHandlers[name], - error: errors[name], - hasChanged: values[name] !== initialValues[name], - initialValue: initialValues[name], - }; - } - - return result; - }, [changeHandlers, errors, initialValues, values]); - - return { - fields, - isValid: Object.values(errors).every(error => !error), - hasChanged: Object.entries(values).some( - ([name, value]) => !isEqual(value, initialValues[name]) - ), - save: () => { - setInitialValues(values); - }, - reset: () => { - setValues(initialValues); - setErrors({}); - }, - }; -}; - -/** - * Creates a form context and hooks for a form with a given set of fields to enable type-safe form handling. - */ -export const createForm = < - FormFields extends Record, - FieldErrors extends Record = Record< - keyof FormFields, - string | undefined - >, ->({ - validators, -}: { - validators?: FormValidators; -}) => { - const FormContext = createContext | undefined>( - undefined - ); - - function FormProvider({ - children, - formState, - }: { - children: React.ReactNode; - formState: FormState; - }) { - return {children}; - } - - const useFormField = (name: K) => { - const formState = useContext(FormContext); - if (!formState) { - throw new Error('useFormField must be used within a FormProvider'); - } - - return formState.fields[name]; - }; - - return { - useFormState: ( - config: Omit, 'validators'> - ) => useFormState({...config, validators}), - FormProvider, - useFormField, - }; -}; diff --git a/static/app/views/settings/dynamicSampling/utils/projectSamplingForm.tsx b/static/app/views/settings/dynamicSampling/utils/projectSamplingForm.tsx deleted file mode 100644 index 46b4b8d2911f67..00000000000000 --- a/static/app/views/settings/dynamicSampling/utils/projectSamplingForm.tsx +++ /dev/null @@ -1,35 +0,0 @@ -import {t} from 'sentry/locale'; -import {createForm} from 'sentry/views/settings/dynamicSampling/utils/formContext'; - -type FormFields = { - projectRates: Record; -}; - -type FormErrors = { - projectRates: Record; -}; - -export const projectSamplingForm = createForm({ - validators: { - projectRates: value => { - const errors: Record = {}; - - Object.entries(value).forEach(([projectId, rate]) => { - if (rate === '') { - errors[projectId] = t('This field is required'); - } - - const numericRate = Number(rate); - if (isNaN(numericRate)) { - errors[projectId] = t('Please enter a valid number'); - } - - if (numericRate < 0 || numericRate > 100) { - errors[projectId] = t('Must be between 0% and 100%'); - } - }); - - return Object.keys(errors).length === 0 ? undefined : errors; - }, - }, -}); diff --git a/tests/js/sentry-test/jest-environment.js b/tests/js/sentry-test/jest-environment.js new file mode 100644 index 00000000000000..93083f2ae59491 --- /dev/null +++ b/tests/js/sentry-test/jest-environment.js @@ -0,0 +1,28 @@ +const SentryEnvironment = require('@sentry/jest-environment/jsdom'); + +// @sentry/jest-environment mutates config.projectConfig.testEnvironmentOptions +// .sentryConfig.init in-place (pushing integrations and calling Sentry.init). +// When Jest runs in-band (≤1 test, e.g. via --changedSince), those mutations +// create circular references that crash ScriptTransformer's config serialisation. +// Deep-cloning sentryConfig isolates the mutation from the original config object. +class SafeSentryEnvironment extends SentryEnvironment { + /** @param {import('@jest/environment').JestEnvironmentConfig} config @param {import('@jest/environment').EnvironmentContext} context */ + constructor(config, context) { + const sentryConfig = config.projectConfig.testEnvironmentOptions?.sentryConfig; + if (sentryConfig) { + config = { + ...config, + projectConfig: { + ...config.projectConfig, + testEnvironmentOptions: { + ...config.projectConfig.testEnvironmentOptions, + sentryConfig: structuredClone(sentryConfig), + }, + }, + }; + } + super(config, context); + } +} + +module.exports = SafeSentryEnvironment; diff --git a/tests/js/sentry-test/sentry-jest-environment.d.ts b/tests/js/sentry-test/sentry-jest-environment.d.ts new file mode 100644 index 00000000000000..7ebeac108507d8 --- /dev/null +++ b/tests/js/sentry-test/sentry-jest-environment.d.ts @@ -0,0 +1,7 @@ +declare module '@sentry/jest-environment/jsdom' { + // eslint-disable-next-line import/no-extraneous-dependencies -- transitive dep of jest + import type {JestEnvironment} from '@jest/environment'; + + const SentryEnvironment: typeof JestEnvironment; + export = SentryEnvironment; +} diff --git a/tests/sentry/hybridcloud/apigateway/test_proxy.py b/tests/sentry/hybridcloud/apigateway/test_proxy.py index c1458db1ba4f71..c8a66042dfc74d 100644 --- a/tests/sentry/hybridcloud/apigateway/test_proxy.py +++ b/tests/sentry/hybridcloud/apigateway/test_proxy.py @@ -7,7 +7,7 @@ from django.core.files.uploadedfile import SimpleUploadedFile from django.http import HttpResponse from django.test.client import RequestFactory -from requests.exceptions import Timeout +from requests.exceptions import ConnectionError, Timeout from sentry.api.exceptions import RequestTimeout from sentry.hybridcloud.apigateway.proxy import proxy_request @@ -352,7 +352,58 @@ def test_timeout_records_error(self) -> None: @responses.activate @override_options(CB_ENABLED) - def test_5xx_response_records_error(self) -> None: + def test_connection_error_records_error(self) -> None: + responses.add( + responses.GET, + f"{self.CELL.address}/connect-error", + body=ConnectionError(), + ) + with patch("sentry.hybridcloud.apigateway.proxy.CircuitBreaker") as mock_breaker_class: + mock_breaker = self._make_breaker_mock(allow_request=True) + mock_breaker_class.return_value = mock_breaker + request = RequestFactory().get("http://sentry.io/connect-error") + with pytest.raises(ConnectionError): + proxy_request(request, self.organization.slug, url_name) + mock_breaker.record_error.assert_called_once() + + @responses.activate + @override_options(CB_ENABLED) + def test_connection_error_records_metric(self) -> None: + responses.add( + responses.GET, + f"{self.CELL.address}/connect-error", + body=ConnectionError(), + ) + with patch("sentry.hybridcloud.apigateway.proxy.metrics") as mock_metrics: + request = RequestFactory().get("http://sentry.io/connect-error") + with pytest.raises(ConnectionError): + proxy_request(request, self.organization.slug, url_name) + mock_metrics.incr.assert_any_call( + "apigateway.proxy.connection_error", + tags={"region": self.CELL.name, "url_name": url_name}, + ) + + @responses.activate + @override_options(CB_ENABLED) + def test_504_response_does_record_error(self) -> None: + responses.add( + responses.GET, + f"{self.CELL.address}/server-error", + status=504, + body=json.dumps({"detail": "gateway timeout"}), + content_type="application/json", + ) + with patch("sentry.hybridcloud.apigateway.proxy.CircuitBreaker") as mock_breaker_class: + mock_breaker = self._make_breaker_mock(allow_request=True) + mock_breaker_class.return_value = mock_breaker + request = RequestFactory().get("http://sentry.io/server-error") + resp = proxy_request(request, self.organization.slug, url_name) + assert resp.status_code == 504 + mock_breaker.record_error.assert_called_once() + + @responses.activate + @override_options(CB_ENABLED) + def test_500_response_does_not_record_error(self) -> None: responses.add( responses.GET, f"{self.CELL.address}/server-error", @@ -366,7 +417,7 @@ def test_5xx_response_records_error(self) -> None: request = RequestFactory().get("http://sentry.io/server-error") resp = proxy_request(request, self.organization.slug, url_name) assert resp.status_code == 500 - mock_breaker.record_error.assert_called_once() + mock_breaker.record_error.assert_not_called() @responses.activate @override_options(CB_ENABLED) diff --git a/tests/sentry/releases/endpoints/test_organization_release_assemble.py b/tests/sentry/releases/endpoints/test_organization_release_assemble.py index 710ae4b9f2fd69..6371f945690a0c 100644 --- a/tests/sentry/releases/endpoints/test_organization_release_assemble.py +++ b/tests/sentry/releases/endpoints/test_organization_release_assemble.py @@ -26,6 +26,39 @@ def setUp(self) -> None: args=[self.organization.slug, self.release.version], ) + def test_no_access_to_all_projects(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = False + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = self.create_release( + version="restricted-release.1", + project=project1, + additional_projects=[project2], + ) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-assemble", + args=[org.slug, release.version], + ) + + checksum = sha1(b"1").hexdigest() + response = self.client.post( + url, + data={"checksum": checksum, "chunks": [checksum]}, + ) + + assert response.status_code == 404 + def test_assemble_json_schema(self) -> None: response = self.client.post( self.url, data={"lol": "test"}, HTTP_AUTHORIZATION=f"Bearer {self.token.token}" diff --git a/tests/sentry/releases/endpoints/test_organization_release_file_details.py b/tests/sentry/releases/endpoints/test_organization_release_file_details.py index cd44002e1b83bf..9d222fe3153dac 100644 --- a/tests/sentry/releases/endpoints/test_organization_release_file_details.py +++ b/tests/sentry/releases/endpoints/test_organization_release_file_details.py @@ -120,6 +120,80 @@ def test_archived_with_dist(self) -> None: class ReleaseFileUpdateTest(APITestCase): + def test_no_access_to_all_projects(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = False + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = Release.objects.create(organization_id=org.id, version="1") + release.add_project(project1) + release.add_project(project2) + + releasefile = ReleaseFile.objects.create( + organization_id=org.id, + release_id=release.id, + file=File.objects.create(name="application.js", type="release.file"), + name="http://example.com/application.js", + ) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-file-details", + kwargs={ + "organization_id_or_slug": org.slug, + "version": release.version, + "file_id": releasefile.id, + }, + ) + response = self.client.put(url, {"name": "foobar"}) + + assert response.status_code == 404 + + def test_no_access_to_all_projects_open_membership(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = True + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = Release.objects.create(organization_id=org.id, version="1") + release.add_project(project1) + release.add_project(project2) + + releasefile = ReleaseFile.objects.create( + organization_id=org.id, + release_id=release.id, + file=File.objects.create(name="application.js", type="release.file"), + name="http://example.com/application.js", + ) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-file-details", + kwargs={ + "organization_id_or_slug": org.slug, + "version": release.version, + "file_id": releasefile.id, + }, + ) + response = self.client.put(url, {"name": "foobar"}) + + assert response.status_code == 200 + def test_simple(self) -> None: self.login_as(user=self.user) @@ -155,6 +229,44 @@ def test_simple(self) -> None: class ReleaseFileDeleteTest(APITestCase): + def test_no_access_to_all_projects(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = False + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = Release.objects.create(organization_id=org.id, version="1") + release.add_project(project1) + release.add_project(project2) + + releasefile = ReleaseFile.objects.create( + organization_id=org.id, + release_id=release.id, + file=File.objects.create(name="application.js", type="release.file"), + name="http://example.com/application.js", + ) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-file-details", + kwargs={ + "organization_id_or_slug": org.slug, + "version": release.version, + "file_id": releasefile.id, + }, + ) + response = self.client.delete(url) + + assert response.status_code == 404 + assert ReleaseFile.objects.filter(id=releasefile.id).exists() + def test_simple(self) -> None: self.login_as(user=self.user) diff --git a/tests/sentry/releases/endpoints/test_organization_release_files.py b/tests/sentry/releases/endpoints/test_organization_release_files.py index 73a0d909558d81..01f8a7c58717fb 100644 --- a/tests/sentry/releases/endpoints/test_organization_release_files.py +++ b/tests/sentry/releases/endpoints/test_organization_release_files.py @@ -206,6 +206,44 @@ def test_queries_should_be_narrowing_search(self) -> None: class ReleaseFileCreateTest(APITestCase): + def test_no_access_to_all_projects(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = False + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = Release.objects.create(organization_id=org.id, version="1") + release.add_project(project1) + release.add_project(project2) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-files", + kwargs={ + "organization_id_or_slug": org.slug, + "version": release.version, + }, + ) + response = self.client.post( + url, + { + "name": "http://example.com/application.js", + "file": SimpleUploadedFile( + "application.js", b"function() { }", content_type="application/javascript" + ), + }, + format="multipart", + ) + + assert response.status_code == 404 + def test_simple(self) -> None: project = self.create_project(name="foo") diff --git a/tests/sentry/releases/endpoints/test_release_deploys.py b/tests/sentry/releases/endpoints/test_release_deploys.py index 343c85bf9e85f4..8321c152f73aa9 100644 --- a/tests/sentry/releases/endpoints/test_release_deploys.py +++ b/tests/sentry/releases/endpoints/test_release_deploys.py @@ -152,6 +152,57 @@ def setUp(self) -> None: self.create_member(teams=[team], user=user, organization=self.org) self.login_as(user=user) + def test_no_access_to_all_projects(self) -> None: + self.org.flags.allow_joinleave = False + self.org.save() + + team2 = self.create_team(organization=self.org) + project2 = self.create_project(teams=[team2], organization=self.org) + + release = Release.objects.create(organization_id=self.org.id, version="1", total_deploys=0) + release.add_project(self.project) + release.add_project(project2) + + url = reverse( + "sentry-api-0-organization-release-deploys", + kwargs={ + "organization_id_or_slug": self.org.slug, + "version": release.version, + }, + ) + response = self.client.post( + url, + data={"name": "foo", "environment": "production", "url": "https://www.example.com"}, + ) + + assert response.status_code == 404 + assert not Deploy.objects.filter(release=release).exists() + + def test_no_access_to_all_projects_open_membership(self) -> None: + self.org.flags.allow_joinleave = True + self.org.save() + + team2 = self.create_team(organization=self.org) + project2 = self.create_project(teams=[team2], organization=self.org) + + release = Release.objects.create(organization_id=self.org.id, version="1", total_deploys=0) + release.add_project(self.project) + release.add_project(project2) + + url = reverse( + "sentry-api-0-organization-release-deploys", + kwargs={ + "organization_id_or_slug": self.org.slug, + "version": release.version, + }, + ) + response = self.client.post( + url, + data={"name": "foo", "environment": "production", "url": "https://www.example.com"}, + ) + + assert response.status_code == 201 + def test_simple(self) -> None: release = Release.objects.create(organization_id=self.org.id, version="1", total_deploys=0) release.add_project(self.project) diff --git a/tests/sentry/seer/entrypoints/test_operator.py b/tests/sentry/seer/entrypoints/test_operator.py index fcdb958b6247e5..65e30756886d44 100644 --- a/tests/sentry/seer/entrypoints/test_operator.py +++ b/tests/sentry/seer/entrypoints/test_operator.py @@ -25,6 +25,7 @@ ) from sentry.seer.explorer.client_models import MemoryBlock, Message, RepoPRState, SeerRunState from sentry.sentry_apps.metrics import SentryAppEventType +from sentry.testutils.asserts import assert_failure_metric from sentry.testutils.cases import TestCase @@ -774,7 +775,7 @@ def test_execute_skips_entrypoint_without_access(self, mock_fetch): mock_no_access.has_access.return_value = False mock_has_access = Mock(spec=SeerExplorerEntrypoint) mock_has_access.has_access.return_value = True - cache_payload = {"thread_id": "abc"} + cache_payload = {"thread_id": "abc", "organization_id": self.organization.id} with ( patch.dict( @@ -816,6 +817,28 @@ def test_execute_skips_entrypoint_without_cache(self, mock_fetch): mock_entrypoint_cls.on_explorer_update.assert_not_called() + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @patch("sentry.seer.explorer.client_utils.fetch_run_status") + def test_execute_records_failure_on_org_mismatch(self, mock_fetch, mock_record): + state = self._make_state( + blocks=[ + MemoryBlock( + id="1", + message=Message(role="assistant", content="summary"), + timestamp="2024-01-01T00:00:00Z", + ), + ] + ) + other_org = self.create_organization() + mock_entrypoint_cls = self._execute_with_mock_entrypoint( + mock_fetch, + state, + cache_return_value={"thread_id": "abc", "organization_id": other_org.id}, + ) + + mock_entrypoint_cls.on_explorer_update.assert_not_called() + assert_failure_metric(mock_record, "org_mismatch") + @patch("sentry.seer.explorer.client_utils.fetch_run_status") def test_execute_with_empty_blocks(self, mock_fetch): state = self._make_state(blocks=[]) diff --git a/tests/snuba/api/endpoints/test_organization_trace_item_attributes.py b/tests/snuba/api/endpoints/test_organization_trace_item_attributes.py index ba200dbf33a2ee..91d70a23b29a77 100644 --- a/tests/snuba/api/endpoints/test_organization_trace_item_attributes.py +++ b/tests/snuba/api/endpoints/test_organization_trace_item_attributes.py @@ -2272,7 +2272,9 @@ def test_attribute_values(self) -> None: assert "POST" in values -class OrganizationTraceItemAttributeValidateEndpointTest(APITestCase, SnubaTestCase, SpanTestCase): +class OrganizationTraceItemAttributeValidateEndpointTest( + APITestCase, BaseSpansTestCase, SpanTestCase +): viewname = "sentry-api-0-organization-trace-item-attributes-validate" feature_flags = { "organizations:visibility-explore-view": True, @@ -2283,7 +2285,7 @@ def setUp(self) -> None: self.login_as(user=self.user) self.project = self.create_project() - def do_request(self, payload=None, features=None, **kwargs): + def do_request(self, payload=None, features=None, query_params=None, **kwargs): if features is None: features = self.feature_flags @@ -2292,68 +2294,57 @@ def do_request(self, payload=None, features=None, **kwargs): self.viewname, kwargs={"organization_id_or_slug": self.organization.slug}, ) - return self.client.post(url, payload, format="json", **kwargs) + return self.client.post( + url, payload, format="json", query_params=query_params, **kwargs + ) def test_no_feature(self): response = self.do_request( - payload={ - "itemType": "spans", - "attributes": ["span.duration"], - }, + payload={"attributes": ["span.duration"]}, + query_params={"itemType": "spans"}, features={}, ) assert response.status_code == 404 def test_missing_item_type(self): response = self.do_request( - payload={ - "attributes": ["span.duration"], - }, + payload={"attributes": ["span.duration"]}, ) assert response.status_code == 400 def test_missing_attributes(self): response = self.do_request( - payload={ - "itemType": "spans", - }, + payload={}, + query_params={"itemType": "spans"}, ) assert response.status_code == 400 def test_empty_attributes_list(self): response = self.do_request( - payload={ - "itemType": "spans", - "attributes": [], - }, + payload={"attributes": []}, + query_params={"itemType": "spans"}, ) assert response.status_code == 400 def test_too_many_attributes(self): response = self.do_request( - payload={ - "itemType": "spans", - "attributes": [f"attr{i}" for i in range(101)], - }, + payload={"attributes": [f"attr{i}" for i in range(101)]}, + query_params={"itemType": "spans"}, ) assert response.status_code == 400 def test_unsupported_item_type(self): response = self.do_request( - payload={ - "itemType": "uptime_results", - "attributes": ["some.attr"], - }, + payload={"attributes": ["some.attr"]}, + query_params={"itemType": "uptime_results"}, ) assert response.status_code == 400 assert "Unsupported item type" in response.data["detail"] def test_well_known_attributes(self): response = self.do_request( - payload={ - "itemType": "spans", - "attributes": ["span.duration"], - }, + payload={"attributes": ["span.duration"]}, + query_params={"itemType": "spans"}, ) assert response.status_code == 200 attr = response.data["attributes"]["span.duration"] @@ -2362,52 +2353,100 @@ def test_well_known_attributes(self): def test_virtual_context_attributes(self): response = self.do_request( - payload={ - "itemType": "spans", - "attributes": ["project"], - }, + payload={"attributes": ["project"]}, + query_params={"itemType": "spans"}, ) assert response.status_code == 200 attr = response.data["attributes"]["project"] assert attr["valid"] is True assert attr["type"] == "string" - def test_user_tags(self): + def test_user_tags_not_in_storage(self): response = self.do_request( payload={ - "itemType": "spans", "attributes": [ "my.custom.tag", "tags[x,string]", "tags[numberAttr,number]", - "tags[booleanAttr,boolean]", - ], + ] }, + query_params={"itemType": "spans"}, + ) + assert response.status_code == 200 + for key in ["my.custom.tag", "tags[x,string]", "tags[numberAttr,number]"]: + assert response.data["attributes"][key]["valid"] is False + assert "error" in response.data["attributes"][key] + + def test_user_tags_in_storage(self): + # Existing and nonexistent tags are validated in separate requests because + # the local test Snuba (used in CI) returns empty results for an OrFilter + # containing multiple ExistsFilters when some reference nonexistent + # attributes, even though real Snuba handles it fine. + self.store_segment( + self.project.id, + uuid4().hex, + uuid4().hex, + span_id=uuid4().hex[:16], + organization_id=self.organization.id, + parent_span_id=None, + timestamp=before_now(days=0, minutes=10).replace(microsecond=0), + transaction="foo", + duration=100, + exclusive_time=100, + tags={"my.custom.tag": "hello"}, + ) + + response = self.do_request( + payload={"attributes": ["my.custom.tag"]}, + query_params={"itemType": "spans"}, ) assert response.status_code == 200 tag1 = response.data["attributes"]["my.custom.tag"] assert tag1["valid"] is True assert tag1["type"] == "string" - tag2 = response.data["attributes"]["tags[x,string]"] - assert tag2["valid"] is True - assert tag2["type"] == "string" + response = self.do_request( + payload={"attributes": ["nonexistent.tag"]}, + query_params={"itemType": "spans"}, + ) + assert response.status_code == 200 + tag2 = response.data["attributes"]["nonexistent.tag"] + assert tag2["valid"] is False + assert "error" in tag2 + + def test_user_tags_same_name_different_types(self): + self.store_segment( + self.project.id, + uuid4().hex, + uuid4().hex, + span_id=uuid4().hex[:16], + organization_id=self.organization.id, + parent_span_id=None, + timestamp=before_now(days=0, minutes=10).replace(microsecond=0), + transaction="foo", + duration=100, + exclusive_time=100, + tags={"foo": "hello"}, + ) + + response = self.do_request( + payload={"attributes": ["tags[foo,string]", "tags[foo,number]"]}, + query_params={"itemType": "spans"}, + ) + assert response.status_code == 200 - tag3 = response.data["attributes"]["tags[numberAttr,number]"] - assert tag3["valid"] is True - assert tag3["type"] == "number" + attrs = response.data["attributes"] + assert attrs["tags[foo,string]"]["valid"] is True + assert attrs["tags[foo,string]"]["type"] == "string" - tag4 = response.data["attributes"]["tags[booleanAttr,boolean]"] - assert tag4["valid"] is True - assert tag4["type"] == "boolean" + assert attrs["tags[foo,number]"]["valid"] is False + assert "error" in attrs["tags[foo,number]"] def test_invalid_attributes(self): long_attr = "a" * 201 response = self.do_request( - payload={ - "itemType": "spans", - "attributes": [long_attr, "tags[foo,faketype]"], - }, + payload={"attributes": [long_attr, "tags[foo,faketype]"]}, + query_params={"itemType": "spans"}, ) assert response.status_code == 200 @@ -2418,17 +2457,36 @@ def test_invalid_attributes(self): assert "error" in response.data["attributes"]["tags[foo,faketype]"] def test_mixed_valid_and_invalid(self): + # Existing and nonexistent tags are validated in separate requests because + # the local test Snuba (used in CI) returns empty results for an OrFilter + # containing multiple ExistsFilters when some reference nonexistent + # attributes, even though real Snuba handles it fine. + self.store_segment( + self.project.id, + uuid4().hex, + uuid4().hex, + span_id=uuid4().hex[:16], + organization_id=self.organization.id, + parent_span_id=None, + timestamp=before_now(days=0, minutes=10).replace(microsecond=0), + transaction="foo", + duration=100, + exclusive_time=100, + tags={"my.custom.tag": "hello"}, + ) + long_attr = "a" * 201 + response = self.do_request( payload={ - "itemType": "spans", "attributes": [ "span.duration", "project", "my.custom.tag", long_attr, - ], + ] }, + query_params={"itemType": "spans"}, ) assert response.status_code == 200 attrs = response.data["attributes"] @@ -2444,3 +2502,43 @@ def test_mixed_valid_and_invalid(self): assert attrs[long_attr]["valid"] is False assert "error" in attrs[long_attr] + + response = self.do_request( + payload={"attributes": ["nonexistent.tag"]}, + query_params={"itemType": "spans"}, + ) + assert response.status_code == 200 + attrs = response.data["attributes"] + assert attrs["nonexistent.tag"]["valid"] is False + assert "error" in attrs["nonexistent.tag"] + + def test_stats_period_limits_time_range(self): + self.store_segment( + self.project.id, + uuid4().hex, + uuid4().hex, + span_id=uuid4().hex[:16], + organization_id=self.organization.id, + parent_span_id=None, + timestamp=before_now(days=2).replace(microsecond=0), + transaction="foo", + duration=100, + exclusive_time=100, + tags={"old.tag": "hello"}, + ) + + # Wide time range should find the tag + response = self.do_request( + payload={"attributes": ["old.tag"]}, + query_params={"itemType": "spans", "statsPeriod": "7d"}, + ) + assert response.status_code == 200 + assert response.data["attributes"]["old.tag"]["valid"] is True + + # Narrow time range should not find the tag + response = self.do_request( + payload={"attributes": ["old.tag"]}, + query_params={"itemType": "spans", "statsPeriod": "1h"}, + ) + assert response.status_code == 200 + assert response.data["attributes"]["old.tag"]["valid"] is False diff --git a/uv.lock b/uv.lock index 32dabf01919c03..0c70205b032d8a 100644 --- a/uv.lock +++ b/uv.lock @@ -1418,12 +1418,15 @@ wheels = [ [[package]] name = "orjson" -version = "3.10.10" +version = "3.11.6" source = { registry = "https://pypi.devinfra.sentry.io/simple" } wheels = [ - { url = "https://pypi.devinfra.sentry.io/wheels/orjson-3.10.10-cp313-cp313-macosx_10_15_x86_64.macosx_11_0_arm64.macosx_10_15_universal2.whl", hash = "sha256:44bffae68c291f94ff5a9b4149fe9d1bdd4cd0ff0fb575bcea8351d48db629a1" }, - { url = "https://pypi.devinfra.sentry.io/wheels/orjson-3.10.10-cp313-cp313-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:e27b4c6437315df3024f0835887127dac2a0a3ff643500ec27088d2588fa5ae1" }, - { url = "https://pypi.devinfra.sentry.io/wheels/orjson-3.10.10-cp313-cp313-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:bca84df16d6b49325a4084fd8b2fe2229cb415e15c46c529f868c3387bb1339d" }, + { url = "https://pypi.devinfra.sentry.io/wheels/orjson-3.11.6-cp313-cp313-macosx_15_0_arm64.whl", hash = "sha256:2c6b81f47b13dac2caa5d20fbc953c75eb802543abf48403a4703ed3bff225f0" }, + { url = "https://pypi.devinfra.sentry.io/wheels/orjson-3.11.6-cp313-cp313-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:647d6d034e463764e86670644bdcaf8e68b076e6e74783383b01085ae9ab334f" }, + { url = "https://pypi.devinfra.sentry.io/wheels/orjson-3.11.6-cp313-cp313-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:cded072b9f65fcfd188aead45efa5bd528ba552add619b3ad2a81f67400ec450" }, + { url = "https://pypi.devinfra.sentry.io/wheels/orjson-3.11.6-cp314-cp314-macosx_15_0_arm64.whl", hash = "sha256:132b0ab2e20c73afa85cf142e547511feb3d2f5b7943468984658f3952b467d4" }, + { url = "https://pypi.devinfra.sentry.io/wheels/orjson-3.11.6-cp314-cp314-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:b376fb05f20a96ec117d47987dd3b39265c635725bda40661b4c5b73b77b5fde" }, + { url = "https://pypi.devinfra.sentry.io/wheels/orjson-3.11.6-cp314-cp314-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6dddf9ba706294906c56ef5150a958317b09aa3a8a48df1c52ccf22ec1907eac" }, ] [[package]]