-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(assisted-query): Add RPCs necessary for issues search #102137
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 #102137 +/- ##
===========================================
- Coverage 81.06% 80.94% -0.13%
===========================================
Files 8748 8752 +4
Lines 391254 389549 -1705
Branches 24693 24693
===========================================
- Hits 317156 315304 -1852
- Misses 73743 73890 +147
Partials 355 355 |
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.
lg, the main things are
- avoiding the "attribute" naming (unless this is what the query agent understands best?)
- only getting last 24h - maybe 7d would be better
- adding built-in issue fields (see code ref) (can do in follow-up, up to you)
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def get_issue_attributes(*, org_id: int, project_ids: list[int]) -> dict[str, Any] | None: |
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.
| def get_issue_attributes(*, org_id: int, project_ids: list[int]) -> dict[str, Any] | None: | |
| def get_issue_filter_keys(*, org_id: int, project_ids: list[int]) -> dict[str, Any] | None: |
I think this or search_keys is more clear - attributes seems like a span-specific term.
As a follow-up there are more "built-in" keys we can add that are hard-coded - see
sentry/static/app/views/issueList/utils/useFetchIssueTags.tsx
Lines 207 to 212 in c33f13b
| const additionalTags = builtInIssuesFields({ | |
| currentTags: renamedTags, | |
| assigneeFieldValues: assignedValues, | |
| bookmarksValues: usernames, | |
| organization, | |
| }); |
For ex assigned, first/last seen, issue type/category
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.
are built in keys handled by the query builder's "strategy", or do we expect to return them here? @aayush-se
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.
We should be returning built in keys since there may be specific values that may vary by org I think.
| Args: | ||
| org_id: Organization ID | ||
| project_ids: List of project IDs to query | ||
| query: Search query string (e.g., "is:unresolved") |
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.
Curious how the agent does the query formatting? I guess that's in the assisted query agent? cc @aayush-se
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.
The actual query/filter is just a string where the agent tries to adhere to the syntactical rules specified in the prompt which it follows pretty well
| # If key already exists, we keep the first one (from events) | ||
| # Otherwise add the issue tag | ||
| if tag.get("key") not in tags_dict: | ||
| tags_dict[tag.get("key")] = tag |
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.
Should we merge totalValues?
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.
hmm but then we would want to merge uniqueValues too, no? and i think that's overly complex
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.
Ah ok fair - on that note does the agent even make use of either? Could maybe return just the keys
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.
Idk, waiting for @aayush-se
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.
totalValues as in the count of occurrences of a specific value? If so we don't need that.
I think it would be simpler if we just returned just the keys as @aliu39 mentioned. Don't need all that context up front as the agent should be making subsequent tool calls
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.
Seems reasonable to me. I think there is probably some stuff that is extra (counts, totalValue) but can follow up based on what ends up being useful
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.
just some nits, I think some of this can likely be cleaned up in this file. overall looks good
| is_values = [ | ||
| "resolved", | ||
| "unresolved", | ||
| "archived", | ||
| "escalating", | ||
| "new", | ||
| "ongoing", | ||
| "regressed", | ||
| "assigned", | ||
| "unassigned", | ||
| "for_review", | ||
| "linked", | ||
| "unlinked", | ||
| ] |
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.
Can we pull this out into a constants file of some sorts? Seems like it is referenced a few times
| priority_values = [ | ||
| PriorityLevel.HIGH.to_str(), | ||
| PriorityLevel.MEDIUM.to_str(), | ||
| PriorityLevel.LOW.to_str(), | ||
| ] |
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.
same here and for actionability, category_values, and attribute keys that have no suggested values available
| built_in_fields.append( | ||
| { | ||
| "key": "is", | ||
| "name": "Status", |
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.
why are you defining this name manually here? fwiw the agent just needs to know the public facing key name to create the query which the key itself should be enough for. Can these built in keys be pulled from the endpoints directly instead of hardcoded?
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.
They unfortunately have to be hardcoded because there is no place on the backend where they can be referenced. But can remove the name field
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
Adds 3 new seer RPC endpoints: - given an org ID and a list of project IDs, gets all available attributes - given an org ID, a list of project IDs, an attribute key, and optionally a substring to match on, gets all available values for that attribute - given an org ID, a list of project IDs, and a query string and other params, calls and returns the response from the issues list endpoint (this will have the same data as the frontend gets)
| flags_resp = client.get( | ||
| auth=api_key, | ||
| user=None, | ||
| path=f"/organizations/{organization.slug}/tags/", |
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.
you should be able to use organization id here, and it is a bit safer since slug is mutable
Line 2359 in 3bb7520
| r"^(?P<organization_id_or_slug>[^/]+)/tags/$", |
| events_resp = client.get( | ||
| auth=api_key, | ||
| user=None, | ||
| path=f"/organizations/{organization.slug}/tags/{attribute_key}/values/", |
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.
same here
Line 2359 in 3bb7520
| r"^(?P<organization_id_or_slug>[^/]+)/tags/$", |
| } | ||
|
|
||
|
|
||
| def get_filter_key_values( |
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.
can probably do the requests in this RPC in a threadpool for parallelization speedup
Adds 3 new seer RPC endpoints: - given an org ID and a list of project IDs, gets all available attributes - given an org ID, a list of project IDs, an attribute key, and optionally a substring to match on, gets all available values for that attribute - given an org ID, a list of project IDs, and a query string and other params, calls and returns the response from the issues list endpoint (this will have the same data as the frontend gets)
Adds 3 new seer RPC endpoints: