Add stat framework to Propolis #147

leftwo · 2022-06-23T19:45:16Z

This is an initial pass at putting some Oximeter stats into propolis, dragging Crucible along for the ride.
Feedback welcome about this strategy for metrics, and how might we improve it.

On instance create, a ProducerRegistry can be created that can be passed to libraries that wish to record stats. This allows Propolis to not have to know what stats a library wishes to register.

A new struct, InstanceMetrics, has been added to Context. This is used for tracking stats for an instance as well as containing the ProducerRegistry that libraries can use to register with Oximeter/Nexus for their own stats.

A new optional metrics field in InstanceEnsureRequest that, if is is present, tells propolis server if it should setup metrics at the provided registration address. The propolis CLI also has this same optional argument.

The CrucibleBackend Volume::construct() now takes a ProducerRegistry that if present can be
used to register metrics with Oximeter.

This requires changes in both crucible and omicron, and will have to be merged accordingly.

In this PR, we are creating a standalone metrics endpoint different from the propolis API.

Things that need completion or additional issues before we can merge this:

Decision on what to do if ensure_instance can't contact Nexus/Oximeter for registration.
Use DNS to determine registration address (updated this to just expect it from sled-agent).
Required changes in Crucible merged to main
More comments on how other libraries can add metrics.
Removal of the cargo patch path to crucible, update crucible git rev.

leftwo · 2022-06-23T19:47:36Z

This won't build in CI because it still has a custom path to Crucible.

leftwo · 2022-06-23T21:00:59Z

From hypervisor huddle:
Would it be possible to make this an optional dependency?

How are the metrics that are recorded being stored in the database? Specifically, would it be "easy" for someone who wanted to look at IO over time (where perhaps there were several power cycles) to be able to read out data and construct a graph. The concern here is to be sure that we don't store our data in the database in a way that causes a common use of the data to be difficult to extract.

bnaecker · 2022-06-23T22:03:56Z

How are the metrics that are recorded being stored in the database?

This is covered in RFD 161. It's a bit complicated, since there are tables for tracking different parts of the timeseries (e.g., fields versus measurements). But the raw data is all stored contiguously in one of the measurement tables. For example, each measurement of the Nexus latency histograms, for one endpoint and response code, are stored ordered by time.

Specifically, would it be "easy" for someone who wanted to look at IO over time (where perhaps there were several power cycles) to be able to read out data and construct a graph. The concern here is to be sure that we don't store our data in the database in a way that causes a common use of the data to be difficult to extract.

Easy is appropriately in scare-quotes there. The short answer is that you can get the raw data now, but there are no tools for anything beyond that. That work has just been scheduled behind other stuff.

You've seen the oxdb tool -- that's basically a CLI to the existing querying infrastructure in oximeter_db::Client. One selects a timeseries (named as target:metric), and then an optional time range and optional sequence of filters on the field values. You'll get back zero or more timeseries, with each timestamped sample. All the processing (filtering, aggregating, etc) would have to be done by the client.

leftwo · 2022-06-29T19:30:05Z

If the DNS names are working for Oximeter, I'll add that in place of the hard coded address that is here now, but other than that, I think the general shape of this is ready for any comments.
The crucible side still needs to land and once that does I'll update this PR

server/src/lib/stats.rs

client/src/api.rs

gjcolombo · 2022-06-30T22:10:36Z

propolis/Cargo.toml

@@ -21,7 +21,8 @@ viona_api = { path = "../viona-api" }
 usdt = { version = "0.3.2", default-features = false }
 tokio = { version = "1", features = ["full"] }
 futures = "0.3"
-crucible = { git = "https://github.com/oxidecomputer/crucible", rev = "fed3e8ca7762130ee146fc516a4ef6eed2b91629", optional = true }
+crucible = { git = "https://github.com/oxidecomputer/crucible", branch = "producemetric", optional = true }
+oximeter = { git = "https://github.com/oxidecomputer/omicron", branch = "main", optional = true }


Just to make sure I'm not misreading: it looks like oximeter is only optional if crucible is disabled; otherwise the Crucible module will be missing the ProducerRegistry type. Is that right? (I'm trying to reason through how we might keep this module optional when we extend metrics collection to more than just Crucible.)

I'm actually not 100% sure, but my intent here was trying to allow propolis standalone to build and not have to pull in both crucible and then oximeter.

Crucible needs to know about ProducerRegistry, and now propolis-server will also need to know about it, even if Crucible is not present.

server/src/lib/stats.rs

server/src/lib/server.rs

propolis/src/block/crucible.rs

Control of metric server is now determined at propolis-server start time, though the actual metrics endpoint won't start until the instance itself is started. An optional metric_addr arg if provided will indicate that a metrics endpoint will later be started. Remove metric field from InstanceProperties Updated smf manifest to support metric_addr config option.

No need to wrap things in Arc Mutex, and I don't have to keep track of producer_register outside of instance_ensure().

gjcolombo

Looks great from my end--thanks for the changes!

leftwo · 2022-07-02T02:23:18Z

This now points to the latest crucible rev in main, where all required changes are.

pfmooney · 2022-07-11T17:19:51Z

standalone/Cargo.toml

@@ -27,3 +27,4 @@ slog-term = "2.7"
 default = ["dtrace-probes"]
 dtrace-probes = ["propolis/dtrace-probes"]
 crucible = ["propolis/crucible"]


Now that the crucible backend is using oximeter, shouldn't the feature list propolis/oximeter itself here?

pfmooney · 2022-07-11T17:22:54Z

propolis/Cargo.toml

+crucible = { git = "https://github.com/oxidecomputer/crucible", rev = "2add0de8489f1d4de901bfe98fc28b0a6efcc3ea", optional = true }
+oximeter = { git = "https://github.com/oxidecomputer/omicron", branch = "main", optional = true }


Since these are getting long, maybe define them as [dependencies.crucible] style sections?

leftwo · 2022-07-12T00:35:36Z

@gjcolombo I merged with your changes like we discussed in chat. Let me know if that looks good.

I tested these changes and stats are still produced and oximeter collects them.

gjcolombo · 2022-07-12T17:33:32Z

Looks good to me!

Add stat framework to Propolis

eb867a4

cargo fmt fixes

56b6352

Make oximeter optional for standalone

d131552

This was referenced Jun 24, 2022

Upstairs disk stats -> Oximeter oxidecomputer/crucible#341

Closed

Metric update for the Upstairs oxidecomputer/crucible#362

Merged

Alan Hanson added 7 commits June 27, 2022 16:02

Include stats.rs file for propolis-server

9adebeb

Merge branch 'master' into metrics

91f19cc

Merge branch 'master' into metrics

20d8023

Point to crucible metric branch

5e9550d

Polishing before PR

62641b4

Just count resets

0bdf5f5

More comments

3c411ac

leftwo marked this pull request as ready for review June 29, 2022 19:28

leftwo requested review from luqmana and pfmooney June 29, 2022 19:30

leftwo changed the title ~~WIP: Add stat framework to Propolis~~ Add stat framework to Propolis Jun 29, 2022

leftwo commented Jun 29, 2022

View reviewed changes

server/src/lib/stats.rs Outdated Show resolved Hide resolved

Alan Hanson added 3 commits June 30, 2022 02:58

Optional metric registration comes from Nexus

8cedc2c

Merge branch 'master' into metrics

da24a50

Merge branch 'master' into metrics

2538f2f

gjcolombo reviewed Jun 30, 2022

View reviewed changes

leftwo mentioned this pull request Jul 1, 2022

Improve on propolis metrics #153

Open

PR comments addressed

395d4db

luqmana reviewed Jul 1, 2022

View reviewed changes

server/src/lib/server.rs Outdated Show resolved Hide resolved

server/src/lib/server.rs Outdated Show resolved Hide resolved

propolis/src/block/crucible.rs Outdated Show resolved Hide resolved

Alan Hanson added 2 commits July 1, 2022 20:22

Remove some Arc Mutex layers.

bc3f45e

No need to wrap things in Arc Mutex, and I don't have to keep track of producer_register outside of instance_ensure().

gjcolombo approved these changes Jul 2, 2022

View reviewed changes

Update Crucible rev

cb57337

leftwo mentioned this pull request Jul 2, 2022

Connect Crucible stats through Propolis to Oximeter oxidecomputer/omicron#1347

Merged

3 tasks

iliana mentioned this pull request Jul 2, 2022

add disk metrics endpoint oxidecomputer/omicron#1348

Merged

3 tasks

Merge branch 'master' into metrics

a9399a8

pfmooney reviewed Jul 11, 2022

View reviewed changes

Alan Hanson added 3 commits July 11, 2022 20:42

PR comments

5f4e280

Merge branch 'master' into metrics

8fb1db6

cargo fmt

d67de50

leftwo merged commit 13e23ff into master Jul 12, 2022

pfmooney deleted the metrics branch May 9, 2023 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stat framework to Propolis #147

Add stat framework to Propolis #147

leftwo commented Jun 23, 2022 •

edited

Loading

leftwo commented Jun 23, 2022

leftwo commented Jun 23, 2022

bnaecker commented Jun 23, 2022

leftwo commented Jun 29, 2022

gjcolombo Jun 30, 2022

leftwo Jul 1, 2022

gjcolombo left a comment

leftwo commented Jul 2, 2022

pfmooney Jul 11, 2022

leftwo Jul 12, 2022

pfmooney Jul 11, 2022

leftwo Jul 12, 2022

leftwo commented Jul 12, 2022

gjcolombo commented Jul 12, 2022

		crucible = { git = "https://github.com/oxidecomputer/crucible", rev = "2add0de8489f1d4de901bfe98fc28b0a6efcc3ea", optional = true }
		oximeter = { git = "https://github.com/oxidecomputer/omicron", branch = "main", optional = true }

Add stat framework to Propolis #147

Add stat framework to Propolis #147

Conversation

leftwo commented Jun 23, 2022 • edited Loading

leftwo commented Jun 23, 2022

leftwo commented Jun 23, 2022

bnaecker commented Jun 23, 2022

leftwo commented Jun 29, 2022

gjcolombo Jun 30, 2022

Choose a reason for hiding this comment

leftwo Jul 1, 2022

Choose a reason for hiding this comment

gjcolombo left a comment

Choose a reason for hiding this comment

leftwo commented Jul 2, 2022

pfmooney Jul 11, 2022

Choose a reason for hiding this comment

leftwo Jul 12, 2022

Choose a reason for hiding this comment

pfmooney Jul 11, 2022

Choose a reason for hiding this comment

leftwo Jul 12, 2022

Choose a reason for hiding this comment

leftwo commented Jul 12, 2022

gjcolombo commented Jul 12, 2022

leftwo commented Jun 23, 2022 •

edited

Loading