Repository navigation
feat(search-attributes): add attribute list as default behaviour - #1913
nikkikapadia wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![sentry-junior[bot]](https://avatars.githubusercontent.com/in/2960336?s=60&v=4){kind=small}
There was a problem hiding this comment.
The list path looks solid overall. Requiring a dataset, putting the recommended fields first, and adding the "Showing N of M" note all make sense, and the bot feedback (the provider warning on the no-query path, limit/description drift, the double period) looks addressed. Two things before this lands:
- In
format.ts,describeCellflattens and truncatesbriefbut addsexamplesraw. Some sentry-conventions examples have embedded newlines.exception.stacktrace, which is common on logs and spans, has one, and a newline in a cell ends the markdown row early and breaks the table. Others are long JSON blobs:aws.dynamodb.consumed_capacityis ~500 chars andgen_ai.tool.definitions~355, and the 255-char cap doesn't apply to them. Flatten the examples too, and cap the examples (or the whole cell) the same way you capbrief. While you're there, examples are dropped when an attribute has nobrief, but the hint asks the model to show examples, so it's probably worth emitting them on their own in that case. - The "lists attributes with a note when the AI provider fails" test never reaches
generateText, so the rejected mock is never used and the test passes whether or not provider failure is handled. I'd make it anit.todo(or drop it) until search lands, along with the scaffolding that only it would need:mockAgentResponse, the@ai-sdk/openaimock, and the unusedTag/withProviderFallbackimports insearch.ts. As written, it reads like failure handling is covered when it isn't.
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Looks great to me, as a non-toolkit-y person!
| import type { Tag, TraceItemAttribute } from "../../../api-client"; | ||
| import { UserInputError } from "../../../errors"; | ||
| import { hasAgentProvider } from "../../../internal/agents/provider-factory"; | ||
| import { withProviderFallback } from "../../../internal/agents/provider-fallback"; |
There was a problem hiding this comment.
| import { withProviderFallback } from "../../../internal/agents/provider-fallback"; |
Non-blocking nit: some unused code. I'm surprised this repo doesn't lint for it yet... I'll send a PR.
| const lines: string[] = []; | ||
|
|
||
| if (importantInfo) { | ||
| importantInfo.map((info) => lines.push(info)); |
There was a problem hiding this comment.
[Refactor] Nit: .map conveys mapping info. Using it instead of .forEach like this is a little odd. https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/no-unused-builtin-method-return.md
...but also, this can be a little cleaner:
| importantInfo.map((info) => lines.push(info)); | |
| lines.push(...importantInfo); |
| const labels = DATASET_LABELS[dataset]; | ||
| const lines: string[] = []; | ||
|
|
||
| if (importantInfo) { |
There was a problem hiding this comment.
[Refactor] importantInfo is always defined, since it defaults to []. I'll send a PR for the missing lint rules that would catch things like this. https://typescript-eslint.io/rules/no-unnecessary-condition & co
| if (importantInfo) { |
There was a problem hiding this comment.
[Style] A bunch of functions in this file manually craft strings from lines. In the past I've enjoyed using a generator pattern for that to make it a bit easier to write. Totally not a request for changes, just a "hey you might like this or might not": e21adfc8 -> nikki/search-attributes/list-attr-table...josh/search-attributes-format-generator. Enjoy 😄
There was a problem hiding this comment.
oooo i def like this, I'm gonna do this in a following PR once i have the full tool done 😌 Thanks Josh!
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3b2e096. Configure here.
| { statsPeriod: period }, | ||
| { context: true }, | ||
| ); | ||
| const rows = Object.values(attributes) as TraceItemAttribute[]; |
There was a problem hiding this comment.
Same-name attribute types get collapsed
Medium Severity
fetchCustomAttributes stores attributes in a map keyed only by name, so Object.values keeps a single row when the attributes API returns the same key with more than one type. The table then shows one type and omits the other, so callers can miss a valid typed attribute.
Reviewed by Cursor Bugbot for commit 3b2e096. Configure here.
There was a problem hiding this comment.
we don't really have this issue because custom attributes would be defined like tags[attribute,string] as the tag. The sentry defined ones wouldn't have duplicate keys like this suggests so we can ignore this
Contributes to EXP-1353
The default behaviour to search-attributes would be a table of attributes to show users who don't pass in a query. I've built out that table response. For all cases right now it will return an explanation message and then just a list of attributes. My next pr will be implementing the querying and narrowing down attributes.