Add labeled metric types #97
Conversation
Dexterp37
force-pushed
the
labeled
branch
5 times, most recently
from
264eb36 to
5b35e84
Compare
brizental
left a comment
brizental
left a comment
There was a problem hiding this comment.
It is Friday EOD and I probably missing something in this review. Please consider this a "first pass" and I'll take a closer look on Monday 🧇
glean/src/core/metrics/index.ts
Outdated
| * whatever is stored internally, without performing any specific | ||
| * validation. | ||
| */ | ||
| export class PassthroughMetric extends Metric<JSONValue, JSONValue> { |
There was a problem hiding this comment.
Reminder to self: I don't understand why we need the PassthorughMetric at all.
There was a problem hiding this comment.
Yeah I get it now. I was wrong when I assumed Labeled metrics did not need a Metric declaration. Even though we don't care about their internal representation, they need a Metric representation for generating the ping payload. I am sorry for the back and forth here, but I would prefer if instead of PassthrougMetric you made this LabeledMetric.
There was a problem hiding this comment.
Mh, too bad :) Now, I could do that and move it back to the labeled metric implementation file. Unfortunately doing so will trigger a circular dependency that breaks the TS build :(
There was a problem hiding this comment.
So, if you move this line to somewhere after you define the Metric abstract class the error goes away.
There was a problem hiding this comment.
I renamed this as requested and added a better comment.
This internally changes the id getter on the `MetricType` to read from the store in case of dyanmic labels, as done in the Glean SDK. This is required in order to attempt recording to Glean metrics before it gets initialized.
This supports string, boolean and counter labeled types and ports over tests from the Kotlin implementation in the SDK and the glean-core internals.
brizental
left a comment
brizental
left a comment
There was a problem hiding this comment.
Good to go, assuming the PassthroughMetric comments are addressed.
| * The only internal event triggered at this point is the `afterPingCollection` event, which is triggered after ping collection and logging, and before ping storing. | ||
| * Plugins are built to listen to a specific Glean event. Each plugin must define an `action`, which is executed everytime the event they are listening to is triggered. | ||
| * [#101](https://github.com/mozilla/glean.js/pull/101): BUGFIX: Only validate Debug View Tag and Source Tags when they are present. | ||
| * [#101](https://github.com/mozilla/glean.js/pull/101): BUGFIX: Only validate Debug View Tag and Source Tags when they are present. |
There was a problem hiding this comment.
Why do we have two entries here... Whathever, can be fixed when we make the next release.
2 participants
This introduces the labelled metric types for string, boolean and counter types. The implementation closely follows what's in the Glean SDK, with no major deviation.
Note that this required adding a couple more functions to the database.
TODO: