chore(metrics): Reorganize Metric data type fields

[bruceg]

chore(metrics): Reorganize Metric data type fields

The Metric type is composed of two types of fields:

1. "key" fields which identify which data series the metric belongs to,
2. "data" fields which contain a single data point.

This change splits struct Metric into two parts to allow for these
types of fields to be used separately in maps or arrays. Further, the
series data is split into the name and tags for further breakdown.

This will be rebased once #6034 is merged.

Signed-off-by: Bruce Guenter bruce@timber.io

Closes #6050

[lukesteensen]

My only real concern here is that this is a lot of args for a constructor (i.e. None, None, None), which can make it more difficult to understand at a glance. It's not a huge deal, but if you see any common patterns it could be worth adding a couple of more focused constructors. The other option would be a builder of some kind, but I'm not sure it's worth that yet.

[bruceg]

I know, that constructor is ugly. I put it in as a convenience for converting code instead of writing an even uglier multi-level struct instantiation all over the place:

Metric {
    series: MetricSeries {
        name: MetricName {
            name: "test metric".to_string(),
            namespace: None,
        },
        tags: None,
    },
    data: MetricData {
        timestamp: None,
        kind: MetricKind::Incremental,
        value: MetricValue::Counter { value: 1.0 },
    },
}

I'm open to ideas on how to make it better. Note that doing so means rewriting over 300 call sites. 😢

[lukesteensen]

This seems like a good time to plug https://comby.dev/ or IntelliJ's structural search/replace 😄

I definitely think that we should keep using some kind of constructor or builder just to keep us decoupled from the internal representation. Since you always need a name, kind, and value, one option would be to have a constructor for those and then builder-style with_{namespace,timestamp,tags} methods that consume and return the metric and can be chained. Another option would be to see how many combinations there are and just add a couple of additional constructors to cover the majority of them.

If you start with the most specific pattern you're replacing (i.e. three Nones, then two, then one), you should be able to do a transformation like that in a pretty automated way.

[MOZGIII]

Looks like a good fit for the derive_builder crate

[bruceg]

I reduced the fn new parameters down to the minimum non-optional values, and added builder-like functions for the other three, and it dropped almost 400 lines of code after formatting. That's quite the win. Thanks for pointing me to comby!

[pablosichert]

Why is the reordering necessary here? Is it possibly flaky?

[bruceg]

This test is dependant on the exact ordering of the fields in the output JSON. That field order is in turn dependant on the order serde visits the struct fields, which has now changed. However, readers of JSON should not care what order the fields come in, so it's a harmless change in the output.

[pablosichert]

Some unit (or docs tests) to cover the behavior which is described here would be nice.

[bruceg]

This is going to be changing in a follow-up to this work, which is the whole motivation for this change. Metric buffering should only be keying on the MetricSeries part, but that requires structural changes to the Metrics buffer.