Repository navigation
feat(cli): add attribute list command - #1559
lzhao-sentry wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
BYK
left a comment
There was a problem hiding this comment.
Pre-emptively blocking to do a quick design review (mostly UX)
| # Attributes seen on the same spans as the specified attribute | ||
| sentry attribute list --with gen_ai.request.model | ||
|
|
||
| # Add descriptions, example values, and deprecation info |
There was a problem hiding this comment.
| # Add descriptions, example values, and deprecation info | |
| # Show descriptions, example values, and deprecation info |
| # Attributes seen on the same spans as the specified attribute | ||
| sentry attribute list --with gen_ai.request.model | ||
|
|
||
| # Add descriptions, example values, and deprecation info |
There was a problem hiding this comment.
| # Add descriptions, example values, and deprecation info | |
| # Show descriptions, example values, and deprecation info |
BYK
left a comment
There was a problem hiding this comment.
There's a bit too much slop in this PR, not sure if the agent followed the repo conventions. It also duplicated a lot of existing functionality.
Finally, I'd like to understand why this API is such a snowflake and how can we align its query and pagination system with the rest. CLI can (and should) normalize the pagination so provide a unified and consistent interface but ideally this should already be done on the API side.
| const DEFAULT_DATASET = "spans"; | ||
|
|
||
| /** Default window — matches the MCP server's attribute discovery. */ | ||
| const DEFAULT_PERIOD = "14d"; |
There was a problem hiding this comment.
I think this should match the default period for the rest of the CLI endpoints
There was a problem hiding this comment.
Yep, 7d. The shared default is LIST_PERIOD_FLAG in packages/cli/src/lib/list-command.ts (default: "7d"). span list, trace list, replay list, the event list commands, and agent-conversation list all use it.
Simplest fix: use the shared flag and drop the local constant:
import { LIST_PERIOD_FLAG, PERIOD_ALIASES, /* ... */ } from "../../lib/list-command.js";
// flags:
period: LIST_PERIOD_FLAG,That way, if the CLI-wide default changes, this command changes with it.
Not every command uses 7d. log list defaults to 30d (14d in trace mode), feedback list and trace logs use 14d, issue list and release list use 90d, and explore uses 24h. Still, span list and trace list are the closest siblings to attribute discovery, and they're 7d. Anyone who wants the MCP-style 14d window can pass --period 14d.
There was a problem hiding this comment.
Nvm ignore my comment, I found it.
| const DEFAULT_PERIOD = "14d"; | ||
|
|
||
| /** User-facing dataset names (and aliases) that map to API dataset names. */ | ||
| const DATASET_ALIASES: Record<string, TraceItemAttributeDataset> = { |
There was a problem hiding this comment.
I think we had this mapping somewhere else already? Probably in explore? We should use a single, unified list.
There was a problem hiding this comment.
The issue with using the explore mapping (and by extension some of the helpers) is that errors and replays are included. They don't use attributes, but tags
There was a problem hiding this comment.
Fair point. explore's map shouldn't be reused as-is, since errors and replays use tags, not attributes. That's an argument for sharing it differently, not for keeping a second copy, though. The two copies have already drifted: this PR accepts metric and tracemetrics as aliases, and explore doesn't.
Suggestion: move the dataset aliases into a shared module (e.g. lib/datasets.ts). It would hold one alias table plus a list of which datasets are trace-item datasets, and both commands would build their parsing and validation from it:
// lib/datasets.ts
const DATASET_ALIASES = {
errors: "errors", error: "errors",
spans: "spans", span: "spans",
logs: "logs", log: "logs",
metrics: "tracemetrics", metric: "tracemetrics", tracemetrics: "tracemetrics",
replays: "replays", replay: "replays",
} as const;
/** Datasets backed by trace items, which have attributes rather than tags. */
export const TRACE_ITEM_DATASETS = ["spans", "logs", "metrics"] as const;
export const EXPLORE_DATASETS = ["errors", ...TRACE_ITEM_DATASETS, "replays"] as const;
export function buildDatasetParser<T extends readonly string[]>(allowed: T) { /* resolve alias, validate against allowed, throw ValidationError */ }
export function toUserDataset(apiName: string): string { /* reverse map for hints */ }Then attribute list uses buildDatasetParser(TRACE_ITEM_DATASETS) and explore uses buildDatasetParser(EXPLORE_DATASETS). Both get the same aliases, the same error wording, and the same reverse mapping for hints. explore would also pick up the metric and tracemetrics aliases for free.
The other duplicated pieces I flagged (the type list, the parse/validate helpers) can live in the same module, or in a sibling module next to the existing trace-item API types.
There was a problem hiding this comment.
We could consolidate tags into this command? But I think the endgoal is to eventually only use attributes, and for the scope of the project this command falls under, we're focusing on the datasets that currently use attributes
|
|
||
| /** API dataset name → canonical user-facing name, for hints. */ | ||
| const API_TO_USER_DATASET: Record<string, string> = { | ||
| spans: "spans", |
| * Parse and validate `--dataset`. Accepts canonical names and singular aliases. | ||
| * @internal Exported for testing | ||
| */ | ||
| export function parseDataset(value: string): TraceItemAttributeDataset { |
There was a problem hiding this comment.
Again, all this is probably shared stuff between explore
| * Split a comma-separated key list, keeping commas inside brackets so typed | ||
| * keys like `tags[cart.total,number]` stay whole. | ||
| */ | ||
| function splitKeyList(value: string): string[] { |
There was a problem hiding this comment.
Same as above, pretty sure we do this somewhere else.
| * deduplicated list of attribute keys. | ||
| * @internal Exported for testing | ||
| */ | ||
| export function parseWithKeys( |
There was a problem hiding this comment.
I don't really understand the --with flag. It feels like a filter? We should standardise these. Also I think a better way might be to adopt what we are doing in #1575
We tend to stay away from flags and prefer positional arguments when it makes sense and I think here sentry attribute list org/ <filter> or just sentry attribute list <filter> feels more natural rather than having a --with or even --query flag.
There was a problem hiding this comment.
For --with, it takes attribute keys only and ignores values. So passing span.op will get all other attributes that occur with span.op, given that that span.op had any value. I think it's unique to attributes, so not sure how it would be best to handle it
I do think --search would fit better in sentry attribute list <filter> since it's substring matches for attribute keys
| } | ||
|
|
||
| /** Shell-quote a key for a copyable command (zsh globs `tags[...]`). */ | ||
| function shellKey(key: string): string { |
There was a problem hiding this comment.
Again, this is existing functionality.
| } | ||
| const { key } = example; | ||
| if (dataset === "tracemetrics") { | ||
| return `Tip: Group or filter metrics with 'sentry explore ${org}/ --dataset metrics' (-F to group, -q to filter)`; |
| "Target specification:\n" + | ||
| " sentry attribute list # auto-detect org from DSN or config\n" + | ||
| " sentry attribute list <org>/ # all projects in the organization\n" + | ||
| " sentry attribute list <org>/<project> # one project\n" + | ||
| " sentry attribute list <project> # find project across all orgs\n\n" + | ||
| "Examples:\n" + | ||
| " sentry attribute list --dataset logs --with http.method # Attributes seen on logs with http.method\n" + | ||
| " sentry attribute list --type string --search http # Attributes containing http and are type string\n" + | ||
| " sentry attribute list --period 24h # Get attributes that appeared in the last 24 hours\n" + | ||
| " sentry attribute list --context # Display attribute metadata information\n" + | ||
| " sentry attribute list --json --fields key,type # Output in JSON format with specified fields\n\n" + | ||
| "Alias: `sentry attributes` → `sentry attribute list`", |
There was a problem hiding this comment.
This is not useful help. We need to tell what "attributes" are, where and how they might be useful. All this information, alias, examples etc has dedicated fields and the description/help/skills get auto generated from those. This is hacking and bypassing all that structured way.
There was a problem hiding this comment.
I re-reviewed at 2ebde76 against the earlier threads. Good progress. Fixed so far:
--periodnow usesLIST_PERIOD_FLAG(7d).splitKeyListis gone in favor ofparseFieldsList, which now keeps commas inside brackets and has a test.shellKeyis gone in favor ofshellQuote.- The metrics tip is now a concrete
sentry explore … -m <metric> -F <key>command, and the help now starts by saying what attributes are.
Still open:
- Examples are still hand-written.
fullDescriptionhas its ownExamples:block, andfragments/commands/attribute.mdhas a## Examplessection. The "Generated Docs & Skills" section ofpackages/cli/AGENTS.mdsays examples belong indocs.examples({ description, command }[], seeagent-conversation/list.ts) so they show up in--help, the docs and the skill references. It also says fragments are only for prose the command metadata can't hold. Move both example lists intodocs.examplesand keep only the "Using Attribute Keys" prose in the fragment. - Dataset handling is still a copy of
explore's.DATASET_ALIASES,VALID_DATASETS,API_TO_USER_DATASETandparseDatasetall exist inexplore.tstoo (around L92–122 and L168). The copies have already drifted:metric/tracemetricswork here but not inexplore. I suggested a shared module with a trace-item subset in the L57 thread. That keeps this command limited to spans/logs/metrics without keeping a second copy. --withand--searchare unchanged. That discussion is still going in the L145 thread. One more data point:buildWithQueryalready turns--with kintohas:k. Acceptinghas:<key>in a positional filter would therefore send the API the same query without a new flag. A plain positional term could replace--search, as long as it matches the way #1575's positional name filter matches.
On pagination: leaving out --cursor matches log list (noCursorFlag), and the comment explains why. Still, the per-type per_page plus trimming the extra look-ahead rows (dropLookAheadRows) works around the API's behavior. That's worth filing against the endpoint so the CLI can eventually page like the other list commands.
I didn't run the tests locally. The latest checks on this commit are green.
|
Correction to my review above: I said the checks on
|
BYK
left a comment
There was a problem hiding this comment.
Please address Junior's comments.
Changes
This PR adds a top level
attribute listcommand to CLI as part of an effort to clean up and expose attributes for both users and agents.Some design choices:
--withinstead of--query. Thequeryparam for attribute API doesn't use Sentry query syntax, rather it's just searching attribute keys that appear with the queried keys on same spans, logs, etc--cursorfor now as the attribute API pagination works differently from other APIs (see comments inlist.ts), so it wouldn't be consistent to add pagination as is. Instead attribute filtering is encouraged with--with/--search/--type