Ungate metrics from TIGER_EXPERIMENTAL + UX improvements - #249
Conversation
There was a problem hiding this comment.
This PR itself looks good to me, but I did have some feedback about the tiger service metrics commands it ungates, which I think we should probably address before they're released publicly.
(Apologies for using this PR as an excuse to review the metrics commands themselves, but I don't think I reviewed the PR that originally added them, so I hadn't ever really looked at them until now. And I think it's going to be much easier to change them now, before anyone is relying on them, rather than later, after they've been released 😅.)
| --bucket-seconds int Aggregation bucket size in seconds (optional; server auto-selects based on the time window when omitted, minimum 60s) | ||
| --filter strings Arbitrary label filter as name=value or name!=value (repeatable) | ||
| --fn string Aggregation function applied per bucket. One of: RATE, INCREASE, SUM, AVG, MIN, MAX, MIN_TOTAL, MAX_TOTAL, COUNT, P50, P90, P99, LAST. Rejected on the timescale_cloud_* resource/qps/connections/jobs metrics; omit to let the server pick the default | ||
| --from string Start of the time window (RFC3339) | ||
| --group-by strings Label key to break the result into one series per distinct value (repeatable). Rejected on the same metrics that reject --fn; omit to collapse into a single series | ||
| -h, --help help for series | ||
| --metric string Metric series name | ||
| -o, --output string Output format (json, yaml, table) | ||
| --role string Filter to a specific instance role (PRIMARY or REPLICA) | ||
| --to string End of the time window (RFC3339) | ||
| ``` |
There was a problem hiding this comment.
Fwiw, I found this command to be somewhat challenging to use, as a human. Three required flags (--from, --to, and --metric) feels like a lot. Would it be possible to have default values for --from/-to? Perhaps --from could default to 24 hours ago, and --to could default to now, or something like that? Or it could even be dependent on the particular metric being queried (though that might be complicated to implement, and could be an enhancement). While the last 24 hours would obviously not be the ideal time range for all use cases, just having some kind of default so that it's easier for people to try the command out without first having to construct two valid RFC3339 dates would be really helpful imo.
Additionally, I noticed that shell completions aren't wired up for the service ID argument or the --metric flag, and they definitely should be. Same for some of the other metrics commands. The first thing I tried to do was tab to complete my service ID, and the second thing I tried to do was tab to complete a --metric name, and neither worked 😭.
There was a problem hiding this comment.
If I'm honest I don't expect people to use the CLI without an agent. We want to introduce it for MCP and it just seems like you need to have the CLI too. Perhaps not?
The commands return hundredths/thousands of dates and numbers so unless it's someone used to data science tools you'd have to copy it to Excel to work with it. We used to have another version of the command that would aggregate into a single number, easier to consume but so far it's not a priority, and the math behind it was not accurate. The calculations need to happen over raw data points, not the ones returned here by the API which are already grouped into buckets.
There was a problem hiding this comment.
If I'm honest I don't expect people to use the CLI without an agent.
Yeah, I understand that. But it's still part of the CLI surface area, and is therefore discoverable by humans, and I think it should therefore be possible to use it as a human, even if that's not very useful (at least currently). A lot of people might just want to try the command to see what it returns, and right now, that's prohibitively difficult imo. Giving some of the parameters reasonable default values (especially the ones that are harder to type otherwise, such as RFC3339 timestamps) would go a long way towards making it easier for humans to try it out, and I don't really think it would negatively impact how agents use it.
We want to introduce it for MCP and it just seems like you need to have the CLI too. Perhaps not?
We definitely want to have the CLI command too, not just the MCP tool, because many people would rather their agents use a CLI than an MCP server.
Long term, I think we should also consider an --output chart format that displays the metrics as ASCII charts in your terminal, or something like that (see this library, for instance). Imo, if we do something like that, these commands would definitely be useful to humans (and who knows, LLMs may find that representation helpful for diagnosing certain types of problems, too)
| ``` | ||
| tiger service metrics details [service-id] [flags] | ||
| ``` | ||
|
|
||
| ### Examples | ||
|
|
||
| ``` | ||
| # Describe a metric | ||
| tiger service metrics details --metric pg_stat_activity_count | ||
|
|
||
| # Get metric details as JSON | ||
| tiger service metrics details --metric pg_stat_activity_count --output json | ||
| ``` |
There was a problem hiding this comment.
Why does this command take a service-id argument? What does it do? The usage text above shows [service-id], but neither of the examples include it. So there's no indication as to why I'd ever pass a service ID, or what the value of doing so would be. I even tested it with a totally invalid service ID, and the command worked fine, which makes it even more confusing (especially when it does look like the service ID is being passed to the API endpoint 😕).
If [service-id] isn't actually required, I think we should remove it so it doesn't confuse people (perhaps the API endpoint itself should change as well?). It's going to be a lot easier to do that now than later, after this has already been released publicly.
Also, if the command didn't take a service ID argument anymore, the metric name could be the positional argument instead, which would feel a lot more natural for a command that just returns a details about a metric, imo.
There was a problem hiding this comment.
Neither project or service params are really used for the endpoints that detail or list available metrics.
We have this in case we need to do gradual rollouts in the future for new metrics, or support different metrics depending on the service.
As an example, currently the available metrics endpoint returns the pgbouncer metrics wether the service has connection pooler enabled or not. In the future we'll probably want to make this more accurate and return only the metrics that are relevant to the service.
There was a problem hiding this comment.
Neither project or service params are really used for the endpoints that detail or list available metrics.
We have this in case we need to do gradual rollouts in the future for new metrics, or support different metrics depending on the service.
Tbh, this feels like a strange decision to me. The fact that the service ID doesn't do anything right now, but is still required as an argument just for some hypothetical future reason, feels like bad UX to me. Real people have to use these commands, and I don't think we should be asking people to go out of their way to input parameters that ostensibly have no impact whatsoever on the results. It's confusing, especially given that it's not explained at all in the command's help text. This pattern also means that you can't browse the available metrics until you create a service, which also doesn't feel right to me, from a product perspective.
Additionally, I'm not even sure it's that useful to "return only the metrics that are relevant to the service". Like, is the downside of having to provide a service ID ever time you call this command really outweighed by the slight increase in accuracy of having it only return metrics that are relevant to that specific service? It feels to me like the service ID should maybe be an optional argument, if we want to support it at all, and that these commands should just return all of the metrics by default.
There was a problem hiding this comment.
@nathanjcochran I opened a new one let's focus on that one first and come back to release after 🙏
I think it includes most of your suggestions except for this one about removing the serviceID requirement. There are always trade offs and I agree the UX is slightly worse from this but we need the API contract to allow us to cover the things I said. These are not just hypotheticals, it's things we know we want to be able to do, and adding a required parameter later on would not be possible while being backwards compatible.
Per timescale/savannah-gateway#2038 (comment): MetricsSeriesRequest.name and MetricDetails.name become metric_name. This repo's openapi.yaml isn't resynced from the gateway's spec yet (still blocked per #249), so this patches just these two fields by hand rather than doing the full resync early -- the eventual resync will pick up the same name once it happens.
409e8d2 to
44355bb
Compare
Splits the shared TIGER_EXPERIMENTAL flag: `service metrics`/`service_metrics_*` graduate and are now always registered, while `service backup`/`service_backups` stays behind the flag since it targets a gateway endpoint still marked x-tigerdata-preview and is under active development on origin/toni/backup-regions. Regenerates docs/cli for the newly-unconditional metrics commands, updates CLAUDE.md's Experimental Feature Gating section, and rewrites the CLI/MCP tests that previously assumed metrics needed the experimental gate.
Metrics are unconditional now, so the bool had no remaining effect.
Not needed for this PR.
44355bb to
45b80c5
Compare
Drops the last x-tigerdata-preview markers on the metrics endpoints and schemas, now that savannah-gateway#2038 merged. Diffed cleanly against gateway's current spec first: the only difference was these 12 leftover markers (this repo's earlier metric_name patch already matched). Regenerated internal/api/client.go via go generate -- doc-comment changes only, no field/signature changes, so no other code needed updating. This clears the last blocker on this PR.
AdrianLC
changed the title
2 participants
Part of PE-826: finalize the MCP release for metrics.
TIGER_EXPERIMENTALcurrently gates two unrelated things: the metrics surface (service metrics/service_metrics_*) and the backups surface (service backup/service_backups). This PR does a surgical split: metrics is now always registered (CLI commands, MCP tools, and the MCP server instructions' metrics mention), while backups stays exactly as gated as it is today.Backups stays behind the flag because it's unrelated to this effort, still targets a preview gateway endpoint, and is under active development on
origin/toni/backup-regions— ungating it here would be out of scope and premature.Also regenerates
docs/cli/for the newly-unconditional metrics commands and updates CLAUDE.md's "Experimental Feature Gating" section.Resynced
openapi.yamlfrom savannah-gateway's master now that savannah-gateway#2038 (droppingx-tigerdata-previewfrom the 3 metrics REST operations + their schemas) has merged. Verified end-to-end against the dev gateway with a build of this branch —service metrics available/details/seriesall work, noTIGER_EXPERIMENTALneeded, andmetric_nameround-trips correctly throughdetails.Follow-up: #251 (merged into this branch), addresses most of the CLI UX feedback from review (rename
available-series→available, default--from/--toonseries, shell completions) — except making the service ID argument optional ondetails/available, which is still an open question.