Integrate derived metrics with metrics.rs

[jkosh44]

The metrics-rs-integration feature allows users to automatically export metrics via the metrics.rs crate. Previously, only base metrics were included, derived metrics were excluded. This commit updates the integration to include derived metrics.

I'm not 100% convinced that this is a good idea, users could just reconstruct the derived metrics on their own wherever those metrics end up getting exported to. However, the implementation was easy enough I thought I'd use the PR as a platform to discuss the idea.

[arielb1]

1. Could you add a test to https://github.com/tokio-rs/tokio-metrics/blob/main/tests/auto_metrics.rs ?
2. Is there a way of making test_no_fields_missing work for derived metrics? I suspect there is not since they are method and not fields, but if there is it would be excellent.

[jkosh44]

1. Could you add a test to https://github.com/tokio-rs/tokio-metrics/blob/main/tests/auto_metrics.rs ?
2. Is there a way of making test_no_fields_missing work for derived metrics? I suspect there is not since they are method and not fields, but if there is it would be excellent.

I'll get on that this week/weekend. I was holding off on writing tests until I got buy in that it was a good idea, but it sounds like we are interested in merging this (once tests are added)?

[arielb1]

I'll get on that this week/weekend. I was holding off on writing tests until I got buy in that it was a good idea, but it sounds like we are interested in merging this (once tests are added)?

Yea, we will be interested in merging this. Great PR!

[jkosh44]

Is there a way of making test_no_fields_missing work for derived metrics? I suspect there is not since they are method and not fields, but if there is it would be excellent.

I have two thoughts here. The first is that we can maintain a const array in the impl of TaskMetrics and RuntimeMetrics that contain the names of all methods. For example,

impl RuntimeMetrics {
    const DERIVED_METRICS: &[&str] = &["busy_ratio"];
    const DERIVED_UNSTABLE_METRICS: &[&str] = &["mean_polls_per_park"];

...
}

Then we could check against those arrays in a test to make sure we aren't missing a derived metric. I don't personally like this approach because we still have to remember to manually update the arrays whenever adding a new method. The only real benefit is that these arrays are physically closer to the methods so it will be harder to forget.

The second idea builds off of the first. We could write an attribute proc macro that walks the AST of a struct and builds a list of all method names, and use those lists similarly as the first idea. It's a bit heavyweight but we'd only have to define it once and then it wouldn't require further maintenance. Unless we wanted to add methods to RuntimeMetrics or TaskMetrics in the future that aren't derived metrics, then we'd need a way to ignore these methods.

Any thoughts?

[arielb1]

Any thoughts?

Since we already have a macro that defines the direct metrics (

tokio-metrics/src/runtime.rs

Line 1570 in a610c22

impl RuntimeMetrics {

), a declarative macro that defines the derived metrics sounds fine.

I would not use a proc macro, this is too small for one.

[jkosh44]

a declarative macro that defines the derived metrics sounds fine.

I just pushed a commit with this approach.

Could you add a test to https://github.com/tokio-rs/tokio-metrics/blob/main/tests/auto_metrics.rs ?

I added tests for both derived runtime and task metrics. I couldn't really figure out a way for the value to be deterministic, so I just tested that the values existed and were not 0.

[jkosh44]

a declarative macro that defines the derived metrics sounds fine.

I just pushed a commit with this approach.

Ignoring whitespace while reviewing makes the diff much more pleasant.

[arielb1]

Your indentation is inconsistent between this and the previous block

[jkosh44]

Do you want me to fix that in a follow up?

[jkosh44]

Ah, never mind, I see that you just did that.