Skip to content
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

Move instance health check timeseries to TOML #6034

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

bnaecker
Copy link
Collaborator

No description provided.

@bnaecker bnaecker requested a review from hawkw July 10, 2024 15:27
Copy link
Member

@hawkw hawkw 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 correct to me!

Now that I'm actually looking at this, I kind of wonder if there's a way to combine this schema with the virtual_machine schema for PVPANIC etc (#6035). Would it make sense to have one virtual_machine schema that can query both the Nexus-emitted health check metrics and the Propolis-emitted PVPANIC metrics, and move stuff like Nexus IDs/addresses onto the check metrics themselves?

Naturally, I think it would make more sense to do that in a follow-up change, rather than here, as I think making the move to a TOML schema should probably be a commit that doesn't introduce any change in behavior, but I figured I'd mention it.

@bnaecker
Copy link
Collaborator Author

Thanks @hawkw! I do agree with you on both accounts -- it'd be nice to have one virtual_machine target which includes both the PVPANIC / vCPU stats and health checks together, and I would like to do that in a later commit. Doing it now would actually run afoul of the schema-consistency checks in oximeter as it collects all the data, since we're moving the source of those fields from a target to a metric!

@bnaecker bnaecker merged commit c9f1ddd into main Jul 10, 2024
19 checks passed
@bnaecker bnaecker deleted the move-instance-check-timeseries-to-toml branch July 10, 2024 17:14
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.

2 participants