1552873 - Implement labeled metrics

Status: RESOLVED FIXED

(Data Platform and Tools :: Glean: SDK, task, P1)

Description

[Jan-Erik Rediger [:janerik]]

We need to implement the missing labeled metrics
This includes:

• Rust implementation
• FFI component
• Kotlin wrapper
• Kotlin-side tests
• Storage engine tests (Rust side)
• Documentation

Caution: We might need to discuss the design first, this type behaves different from other types and re-uses those others

Comment 1

[Jan-Erik Rediger [:janerik]]

Problem:

We want to have labeled metrics.
These are just wrapping existing metric types and are encoded in a specific way, so that the storage knows how to get them out and encode them.
Currently it's encoded in the name: category.metricname/label

Labeled metrics have some logic for creating the label:

1. Static labels - Check that a label is in the set of allowed labels or use __other__.
   • Statically known after metrics.yaml parsing
2. Dynamic labels - At most 16 labels, length limit and regex constraint on labels, __other__ if invalid.

Implementing them on the Rust side is possible the same way it's done in Glean AC: A LabeledMetric<SubMetric> generic type.
LabeledMetric also holds the state over static labels/dynamic seen labels.

1. Problem:

Some data types might have more info than CommonMetricData, e.g. the labels of LabeledMetrics or unit of the datetime metric.

Possible solution: Have these field as Option in CommonMetricData, so we can simply re-use trait methods to create them.

2. Problem:

We will need individual storage for each sub type of LabeledMetric in the FFI component (where instances are stored in a ConcurrentHandleMap).
Otherwise we loose the typ information of what it contains and can't create new instances of new sub metrics.

3. Problem:

On the Kotlin side we need a LabeledMetricType, so that we can generate metrics of that type.
These also need to keep track of the sub metric, in order to return an instance of the proper type on .get() (aka []).
However, currently we can only create a metric instance from all meta data, which means we would need to hold on to the meta data and copy that in every time a new sub metric is created.
Additionally we need to create the proper name for it.

Creating label on the Rust side.

Make the Rust side only create the full name (metricname/label) and return it. The Kotlin side then creates a new instance of the metric type with this name.
This has the downside that we need to copy it back into Kotlin, just so we can pass it back into the Rust part for creating the metric.

Creating label on the Kotlin side.

Don't have a LabeledMetric in Rust at all (for now).
Create full names on the Kotlin side. Create instances of metric types using the labels.
Downside: needs logic and state on the Kotlin side. Additionally we might need to load labels from storage on first access, which needs to happen on the Rust side.

Alternative metric constructors

If we can have an alternative constructor for the metric type, we could create a metric from a handle and create the metrics as part of the labeled metric on the Rust side.

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #1)

1. Problem:

Some data types might have more info than CommonMetricData, e.g. the labels of LabeledMetrics or unit of the datetime metric.

Possible solution: Have these field as Option in CommonMetricData, so we can simply re-use trait methods to create them.

Does this mean that all the metrics specific fields will be in here? Is so, this seems a bit weird :( Would you mind describing the problem here with an example piece of code?

2. Problem:

We will need individual storage for each sub type of LabeledMetric in the FFI component (where instances are stored in a ConcurrentHandleMap).
Otherwise we loose the typ information of what it contains and can't create new instances of new sub metrics.

Can you expand a bit on this? e.g. if we're making a labelled counter, do we need a separate storage for "labelled things", "labelled counters" or "counters that have been used in labelled metrics"?

3. Problem:

On the Kotlin side we need a LabeledMetricType, so that we can generate metrics of that type.
These also need to keep track of the sub metric, in order to return an instance of the proper type on .get() (aka []).
However, currently we can only create a metric instance from all meta data, which means we would need to hold on to the meta data and copy that in every time a new sub metric is created.
Additionally we need to create the proper name for it.

Creating label on the Rust side.

Make the Rust side only create the full name (metricname/label) and return it. The Kotlin side then creates a new instance of the metric type with this name.
This has the downside that we need to copy it back into Kotlin, just so we can pass it back into the Rust part for creating the metric.

While not ideal, this seems good enough to start with.

Creating label on the Kotlin side.

Don't have a LabeledMetric in Rust at all (for now).
Create full names on the Kotlin side. Create instances of metric types using the labels.
Downside: needs logic and state on the Kotlin side. Additionally we might need to load labels from storage on first access, which needs to happen on the Rust side.

I'm not sure I like this one. I'd love the Rust core to be in control of this if possible.

Alternative metric constructors

If we can have an alternative constructor for the metric type, we could create a metric from a handle and create the metrics as part of the labeled metric on the Rust side.

We can have them, tag them as internal (since they would only be available from within LabelledMetricTypes, not as part of the API). Would this make the above proposals not relevant anymore? I wonder what that means for the Swift version too.

Comment 3

[Jan-Erik Rediger [:janerik]]

(In reply to Alessio Placitelli [:Dexter] from comment #2)

Does this mean that all the metrics specific fields will be in here? Is so, this seems a bit weird :( Would you mind describing the problem here with an example piece of code?

Essentially yes.
Right now "constructors" for metric types only take the common meta data, e.g. the counter metric.

This allows us to implement it as a trait and then instantiate new metrics generically.

Now e.g. a TimingDistribution will require additional data (the unit, that is hour/minute/second/...).
Therefore T::with_meta wouldn't work generically anymore.
We would need to implement the creation for each LabeledMetric<T> individually (e.g. LabeledMetric<Counter>, LabeledMetric<String>, ...).
Not impossible but a bit of duplicated logic.

Can you expand a bit on this? e.g. if we're making a labelled counter, do we need a separate storage for "labelled things", "labelled counters" or "counters that have been used in labelled metrics"?

static ref LABELED_COUNTER: ConcurrentHandleMap<LabeledCounter<CounterMetric>> = ConcurrentHandleMap::new();
static ref LABELED_BOOL: ConcurrentHandleMap<LabeledCounter<BooleanMetric>> = ConcurrentHandleMap::new();
...

We meed the sub type (e.g. CounterMetric around) so that we can properly create the metric.

I'm not sure I like this one. I'd love the Rust core to be in control of this if possible.

Yeah, I was also going for having all logic in Rust only. This is the first time I actively explored putting the logic in Kotlin again :/

We can have them, tag them as internal (since they would only be available from within LabelledMetricTypes, not as part of the API). Would this make the above proposals not relevant anymore? I wonder what that means for the Swift version too.

Alternative metric constructors would probably allow us to avoid the other 2 options (as we only need to pass around the handle and that should be cheap, it's an integer).
It still means we have problem 2, the ConcurrentHandleMap storage overhead.

Comment 4

[Jan-Erik Rediger [:janerik]]

Attachment: GitHub Pull Request

Comment 5

[Michael Droettboom [:mdroettboom]]

Thought experiment -- does the "nuclear option" of changing the API buy us a lot in terms of reduced complexity? i.e. instead of a labeled metric as a container type for a concrete metric whose name gets changed, just have new metric types labeled_boolean with set(label, value) rather than metric['label'].set(value).

We'd have to backfill the API usage in the Fenix, not at all ideal, but if this design is killing us trying to shoehorn it into this new model, maybe we bite the bullet?

Comment 6

[Jan-Erik Rediger [:janerik]]

The PR got merged, with some work happening in follow-ups