Skip to content

ref(preprod): Standardize logging key to preprod_artifact_id#115462

Open
NicoHinderling wants to merge 2 commits into
masterfrom
nico/standardize-preprod-artifact-id-logging
Open

ref(preprod): Standardize logging key to preprod_artifact_id#115462
NicoHinderling wants to merge 2 commits into
masterfrom
nico/standardize-preprod-artifact-id-logging

Conversation

@NicoHinderling
Copy link
Copy Markdown
Contributor

Logging and SDK tags inconsistently used both artifact_id and preprod_artifact_id across preprod code, making it difficult to search and filter logs in Sentry (you'd have to query for both key names to find all logs for a given artifact).

This standardizes all logging extra fields, SDK tags, and test assertions to use preprod_artifact_id, which is more specific and avoids ambiguity with other artifact concepts in Sentry (release artifacts, debug artifacts, etc.).

API parameter definitions (name="artifact_id") are left unchanged since they are URL/query parameters, not observability keys.

Companion PR in launchpad: getsentry/launchpad#620

Logging and SDK tags inconsistently used both `artifact_id` and
`preprod_artifact_id` across preprod code, making it harder to search
and filter logs in Sentry. Standardizes all logging extra fields,
SDK tags, and test assertions to use `preprod_artifact_id`.

API parameter definitions (`name="artifact_id"`) are left unchanged
since they are URL/query parameters, not observability keys.
@NicoHinderling NicoHinderling marked this pull request as ready for review May 13, 2026 00:42
@NicoHinderling NicoHinderling requested a review from a team as a code owner May 13, 2026 00:42
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 13, 2026
Comment thread src/sentry/preprod/tasks.py Outdated
"project_id": project.id,
"org_id": org_id,
"artifact_id": artifact_id,
"preprod_artifact_id": artifact_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Task kwarg rename breaks Celery task invocation

High Severity

The apply_async kwargs key was renamed from artifact_id to preprod_artifact_id at the call sites, but the receiving task function signatures still use artifact_id as the parameter name. For compare_preprod_artifact_size_analysis (param artifact_id at line 55) and assemble_preprod_artifact (param artifact_id at line 83), both lack defaults, so Celery will raise a TypeError at runtime. The preprod_artifact_id kwarg gets silently absorbed by **kwargs.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fed8050. Configure here.

"checksum": checksum,
"chunks": chunks,
"artifact_id": head_artifact_id,
"preprod_artifact_id": head_artifact_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Size analysis assembly receives None artifact ID silently

High Severity

The kwarg key was renamed to preprod_artifact_id when calling assemble_preprod_artifact_size_analysis, but the task function parameter is still artifact_id: int | None = None. Since it defaults to None, the task won't crash but will silently process with a None artifact ID, causing downstream failures or data corruption. The same issue affects assemble_preprod_artifact_installable_app, which has no default and will crash.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fed8050. Configure here.

response_data = {
"success": True,
"artifact_id": artifact_id,
"preprod_artifact_id": artifact_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

API response key renamed breaks existing consumers

Medium Severity

The artifact_id key in API Response dicts was renamed to preprod_artifact_id, changing the JSON shape returned to clients. The PR description states that API parameter definitions are left unchanged, yet response body keys were also renamed. Existing tests still assert response.data["artifact_id"] and will break, and any API consumers depending on the artifact_id key in responses will also break.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fed8050. Configure here.

… contracts

The previous commit renamed `artifact_id` to `preprod_artifact_id` in
logging extras but also inadvertently changed apply_async kwargs, API
response dict keys, and event tags. Revert those non-logging changes
to keep task signatures, API contracts, and tag names intact.

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

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

There are 4 total unresolved issues (including 3 from previous reviews).

Fix All in Cursor

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

Reviewed by Cursor Bugbot for commit a76feac. Configure here.

tags = _artifact_to_tags(artifact)

assert tags == {"app_id": "com.example.app", "artifact_id": str(artifact.id)}
assert tags == {"app_id": "com.example.app", "preprod_artifact_id": str(artifact.id)}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test updated but source code key not changed

High Severity

The test assertion was changed to expect "preprod_artifact_id" in the dict returned by _artifact_to_tags, but the actual implementation in grouptype.py line 59 still sets tags["artifact_id"]. Either the source function was not updated to match (missing from this PR), or the test assertion is wrong. This will cause the test to fail, and the standardization goal of the PR is incomplete for SDK tags emitted by this function.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a76feac. Configure here.

@github-actions
Copy link
Copy Markdown
Contributor

Backend Test Failures

Failures on 79f703e in this run:

tests/sentry/preprod/size_analysis/test_size_analysis_tasks.py::ArtifactToTagsTest::test_minimal_artifact_datalog
[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/preprod/size_analysis/test_size_analysis_tasks.py:418: in test_minimal_artifact_data
    assert tags == {"app_id": "com.example.app", "preprod_artifact_id": str(artifact.id)}
E   AssertionError: assert {'app_id': 'c...act_id': '67'} == {'app_id': 'c...act_id': '67'}
E     
E     Omitting 1 identical items, use -vv to show
E     Left contains 1 more item:
E     �[0m{�[33m'�[39;49;00m�[33martifact_id�[39;49;00m�[33m'�[39;49;00m: �[33m'�[39;49;00m�[33m67�[39;49;00m�[33m'�[39;49;00m}�[90m�[39;49;00m
E     Right contains 1 more item:
E     �[0m{�[33m'�[39;49;00m�[33mpreprod_artifact_id�[39;49;00m�[33m'�[39;49;00m: �[33m'�[39;49;00m�[33m67�[39;49;00m�[33m'�[39;49;00m}�[90m�[39;49;00m
E     
E     Full diff:
E     �[0m�[90m �[39;49;00m {�[90m�[39;49;00m
E     �[90m �[39;49;00m     'app_id': 'com.example.app',�[90m�[39;49;00m
E     �[91m-     'preprod_artifact_id': '67',�[39;49;00m�[90m�[39;49;00m
E     ?      --------�[90m�[39;49;00m
E     �[92m+     'artifact_id': '67',�[39;49;00m�[90m�[39;49;00m
E     �[90m �[39;49;00m }�[90m�[39;49;00m

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

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants