oxidecomputer / oxidecomputer/omicron

add new measurement types from #10315 to sp-sim config

Open Beginner friendly
#10,337 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

see https://github.com/oxidecomputer/omicron/pull/10315#discussion_r3139695642:

I think the reason that the test passes even if the new metrics aren't recorded is because the SP simulator isn't configured to emit any of the new metric types in gateway-test-utils/configs/sp_sim_config.test.toml (for example, like this). That would explain why the first two changes (not inserting the new metric types) don't break the test: there aren't any in the config, and the test is asserting that the metrics that are in Oximeter match the sensors that the config file says the SP will have.

As to why commenting out the loop over the configs also passes, that loop is building the expected sensor metrics from the config, and we then assert that we saw all the metrics in Oximeter that were defined in the config. If we don't build the maps of which metrics we expect, we will pass no matter what metrics exist.

I think if we wanted to make the test better at failing in cases where it needs to be updated to include new sensor types as well as cases where the metrics code is actually broken, we could make it panic if it encounters any hardware_component metric that wasn't in the expected maps from the config file. But you would still have to update the config file to make that break. Perhaps we could also have a test that asserts that the config file contains at least one sensor of each type that exists in the MGS API, if we want to be really fastidious about it?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read the discussion in pull request #10315 and inspect gateway-test-utils/configs/sp_sim_config.test.toml, especially the existing sensor entries around lines 190-203. Add the new measurement types from that change to the simulator configuration, then run the relevant gateway or SP simulator metrics test and confirm the configured sensors are all observed.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
testing
Issue type
Feature
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.