Skip to content

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Oct 9, 2025

  • Convert query summaries lazily; h/t @david-crespo.
  • Drop query summaries from user-facing docs; h/t @ahl. This feature is only useful for Oxide engineers to investigate oximeter performance, and we don't want to leak implementation details into public docs.

@jmcarp jmcarp force-pushed the jmcarp/oximeter-query-summaries-cleanup branch from 080f598 to 23d896c Compare October 9, 2025 15:39
Copy link
Contributor

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good with the very significant caveat that I don't understand how we intend to use it or for it to be used.

pub tables: Vec<OxqlTable>,
/// Summaries of queries run against ClickHouse.
/// Summaries of queries run against ClickHouse. Note: we omit this field
//from the generated docs, since it's mainly of interest internally.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@david-crespo
Copy link
Contributor

david-crespo commented Oct 10, 2025

I like skipping it. Didn't realize it was aimed at internal users. We might need to think about this kind of thing more broadly — I can't think of any other spots we hide a field in the OpenAPI schema for this purpose. Would be good to have a sort of policy or rule of thumb about when it's appropriate in order to avoid doing it too much.

* Convert query summaries lazily; h/t @david-crespo.
* Drop query summaries from user-facing docs; h/t @ahl. This feature is only
  useful for Oxide engineers to investigate oximeter performance, and we don't
  want to leak implementation details into public docs.
@jmcarp jmcarp force-pushed the jmcarp/oximeter-query-summaries-cleanup branch from 1a5ac66 to 0818cb6 Compare October 14, 2025 14:22
@jmcarp
Copy link
Contributor Author

jmcarp commented Oct 14, 2025

This looks good with the very significant caveat that I don't understand how we intend to use it or for it to be used.

I think I can get the information I need with oxide api, so I'm happy with omitting these fields from the openapi docs.

@jmcarp jmcarp enabled auto-merge (squash) October 14, 2025 14:41
@jmcarp jmcarp merged commit 77bb62f into main Oct 14, 2025
17 checks passed
@jmcarp jmcarp deleted the jmcarp/oximeter-query-summaries-cleanup branch October 14, 2025 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants