-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(explorer): add support for multiple artifacts in client #104127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #104127 +/- ##
===========================================
- Coverage 80.60% 80.60% -0.01%
===========================================
Files 9324 9324
Lines 398390 398420 +30
Branches 25464 25464
===========================================
+ Hits 321127 321142 +15
- Misses 76809 76824 +15
Partials 454 454 |
src/sentry/seer/explorer/client.py
Outdated
| requests.HTTPError: If the Seer API request fails | ||
| ValueError: If artifact_schema is provided without artifact_key | ||
| """ | ||
| if artifact_schema and not artifact_key: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if artifact_schema and not artifact_key: | |
| if bool(artifact_schema) ^ bool(artifact_key): |
xor
| extra = "allow" | ||
|
|
||
|
|
||
| class ArtifactBlock(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs update to match model in seer
| class ArtifactBlock(BaseModel): | |
| class Artifact(BaseModel): |
+rm schema
## Problem Both in the Replay Details and Span Waterfall, we have this ability to paginate to sibling events. However, the implementations are inconsistent. ### Replay Details <img width="2880" height="2042" alt="CleanShot 2025-11-28 at 14 04 39@2x" src="https://github.com/user-attachments/assets/ff6a81ce-dda5-4db6-b775-a3183a957eaf" /> ### Span Waterfall <img width="2880" height="2042" alt="CleanShot 2025-11-28 at 14 06 48@2x" src="https://github.com/user-attachments/assets/5fda7778-712d-4dfe-aa9b-e2c6a4f5fe2b" /> ## Goal This PR isn't about implementing the pagination UI consistently; rather, it's about modifying the breadcrumbs UI so that we can reach that end goal. Specifically, this PR swaps out the chevron icon for a forward slash (new icon). ### Before <img width="2352" height="277" alt="CleanShot 2025-11-28 at 13 58 09@2x" src="https://github.com/user-attachments/assets/8e4c6638-7785-4060-b0a4-1ba978589989" /> ### After <img width="2351" height="278" alt="CleanShot 2025-11-28 at 13 58 37@2x" src="https://github.com/user-attachments/assets/9ada7a5f-13a9-48dd-824e-e6fdb2fc01a1" /> ## Solution We're making this change because we want to reach an end goal where we have this ### Ideal end state <img width="1978" height="284" alt="CleanShot 2025-11-28 at 14 26 50@2x" src="https://github.com/user-attachments/assets/b3026ca0-3b8d-4898-bf76-933cf74e80b6" /> ### End state without this change <img width="1976" height="286" alt="CleanShot 2025-11-28 at 14 27 33@2x" src="https://github.com/user-attachments/assets/02d49a61-cae7-4db2-85ce-eb77cfedafbf" /> ## Solution Explanation In the realm of computing, the slash is frequently used to indicate a file path.
Redo this change as it was reverted due to gaps in the control silo deployment pipeline that didn't run migrations. This migration will have run in development environment, and once the control silo migration pipeline is fixed, I'll get a new migration number and use a `SeparateDatabaseAndState` migration to handle this scenario. Reapply "feat(cells) Add date_updated to organizationmapping (#103772)" This reverts commit 94e744c. Refs INFRENG-172
1. Add details widget to query summary page 2. Add LOC to query summary 3. Update layout accordingy 4. Update styling to the db query description so it vertically scrolls, cales and looks better Before <img width="1397" height="446" alt="image" src="https://github.com/user-attachments/assets/dfe93222-7946-4a8f-8c56-080885e5d5f8" /> <img width="669" height="490" alt="image" src="https://github.com/user-attachments/assets/f8c1866d-1631-4616-ac1d-42974f933dc3" /> After (it's hard to screenshot this, but there is a scroll bar now) <img width="1218" height="425" alt="image" src="https://github.com/user-attachments/assets/0d92fcf6-330a-4a22-9317-346f74b6c082" /> <img width="636" height="474" alt="image" src="https://github.com/user-attachments/assets/d43a2e3d-669e-4626-9c63-43fe694c38fd" />
#104111) …nge too far When an absolute date range is too far in the past, we should fall back to using the max pickable days.
Some orgs need a higher subscriptions limit, so adding options to enable extending the limits for these orgs.
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
| key: The artifact key | ||
| schema: The Pydantic model class to parse the artifact data into | ||
| Returns: | ||
| The parsed artifact as a typed Pydantic model, or None if not found or not yet generated | ||
| """ | ||
| artifacts = self.get_artifacts() | ||
| artifact = artifacts.get(key) | ||
| if artifact is None or artifact.data is None: | ||
| return None | ||
| return schema.parse_obj(artifact.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: get_artifact() lacks ValidationError handling when parsing artifact.data, leading to crashes on schema mismatches.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The get_artifact() method in client_models.py calls schema.parse_obj(artifact.data) without ValidationError handling. If artifact.data does not conform to the schema, Pydantic's parse_obj() will raise a ValidationError, causing an unhandled crash in the caller and breaking the multi-artifact workflow feature. This is a regression from previous code that gracefully handled such errors.
💡 Suggested Fix
Reintroduce try...except ValidationError around schema.parse_obj(artifact.data) in get_artifact(). On error, set state.artifact = None and capture the exception with sentry_sdk.capture_exception().
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/seer/explorer/client_models.py#L84-L99
Potential issue: The `get_artifact()` method in `client_models.py` calls
`schema.parse_obj(artifact.data)` without `ValidationError` handling. If `artifact.data`
does not conform to the `schema`, Pydantic's `parse_obj()` will raise a
`ValidationError`, causing an unhandled crash in the caller and breaking the
multi-artifact workflow feature. This is a regression from previous code that gracefully
handled such errors.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4725065
| if artifact_schema and not artifact_key: | ||
| raise ValueError("artifact_key is required when artifact_schema is provided") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: continue_run() allows artifact_key without artifact_schema, leading to silent failure of artifact generation.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The continue_run() method in client.py has asymmetric validation for artifact_key and artifact_schema. It prevents artifact_schema without artifact_key, but allows artifact_key without artifact_schema. If artifact_key is provided alone, validation passes, but the artifact_key is silently omitted from the API payload, causing the intended artifact generation to be ignored without error.
💡 Suggested Fix
Modify the validation in continue_run() to be symmetric, requiring both artifact_key and artifact_schema to be provided together, similar to start_run().
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/seer/explorer/client.py#L245-L246
Potential issue: The `continue_run()` method in `client.py` has asymmetric validation
for `artifact_key` and `artifact_schema`. It prevents `artifact_schema` without
`artifact_key`, but allows `artifact_key` without `artifact_schema`. If `artifact_key`
is provided alone, validation passes, but the `artifact_key` is silently omitted from
the API payload, causing the intended artifact generation to be ignored without error.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4725065
| ValueError: If artifact_schema is provided without artifact_key | ||
| """ | ||
| if artifact_schema and not artifact_key: | ||
| raise ValueError("artifact_key is required when artifact_schema is provided") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Asymmetric validation allows artifact_key without artifact_schema
The validation in continue_run only checks if artifact_schema and not artifact_key, which is asymmetric compared to start_run which uses proper XOR logic if bool(artifact_schema) != bool(artifact_key). This allows providing artifact_key without artifact_schema, but the key gets silently ignored since line 260 requires both parameters for inclusion in the payload. The validation should use XOR like start_run to ensure both parameters are provided together or neither.
Additional Locations (1)
|
|
||
| client = SeerExplorerClient(self.organization, self.user) | ||
| with pytest.raises(ValueError, match="artifact_key is required"): | ||
| client.start_run("Analyze", artifact_schema=IssueAnalysis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Test regex pattern won't match actual error message
The test test_start_run_artifact_schema_requires_key expects match="artifact_key is required" but the actual error raised by start_run is "artifact_key and artifact_schema must be provided together". The word "required" doesn't appear in the actual error message, so re.search() will fail and the test will not pass. Either the error message or the test pattern needs to be updated to match.
Allows passing in different artifact models when continuing a run in the Explorer Client, and retrieving different kinds of artifacts that were generated during the run. Enables multi-step/multi-artifact workflows like Autofix
Requires https://github.com/getsentry/seer/pull/4139
Part of AIML-1727