[glean] The `metrics.*` API is technically public, but isn't intended as such
Categories
(Toolkit :: Telemetry, defect, P1)
Tracking
()
People
(Reporter: mdroettboom, Assigned: mdroettboom)
Details
(Whiteboard: [telemetry:mobilesdk:m7])
Attachments
(4 files)
The various *MetricType classes are technically public so that they can be used from the code that glean_parser generates from the metrics.yaml, since it exists in the applications package namespace.
We've already seen some unintended use of these classes in the wild.
This bug is to investigate alternatives and remediations to this problem.
(1) Improve the docs
(2) Use a naming convention that makes their intended use more clear
(3) Some clever way to make them private, but still accessible from the generated code. (Does using inheritance rather than instantiation help?)
|
||
Comment 1•7 years ago
|
||
(In reply to Michael Droettboom [:mdroettboom] from comment #0)
The various
*MetricTypeclasses are technically public so that they can be used from the code that glean_parser generates from the metrics.yaml, since it exists in the applications package namespace.We've already seen some unintended use of these classes in the wild.
One of them being this: https://github.com/mozilla-mobile/fenix/blob/cf0d1355b5844665085687761037a28645530b41/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt#L7
This bug is to investigate alternatives and remediations to this problem.
(1) Improve the docs
(2) Use a naming convention that makes their intended use more clear
I think the above are proper solution we could enable in the short term.
(3) Some clever way to make them private, but still accessible from the generated code. (Does using inheritance rather than instantiation help?)
This might be a rabbit hole :( We could probably break this bug in 2: handling (1) and (2) here, having the investigation for (3) in a follow-up. I feel like (1)/(2) are important enough to deal with them before other people start relying on internals.
|
Assignee | |
Updated•7 years ago
|
|
|
Assignee | |
Comment 2•7 years ago
|
||
|
|
Assignee | |
Comment 3•7 years ago
|
||
Great. The plan of attack here is to submit and merge a PR against Fenix to remove this unintended usage. Once we aren't at risk of breaking Fenix, we can attack the changes to the naming convention.
|
|
Assignee | |
Comment 4•7 years ago
|
||
The changes to Fenix have been merged, so we're free to monkey around with this "not-so-public" API.
|
|
Assignee | |
Comment 5•7 years ago
|
||
So, brainstorm time.
The docs part should be obvious -- we just need to add a section to the metrics docs somewhere.
As for any changes in the code: We could move the glean.metrics namespace to glean.PRIVATE?
There's perhaps more complex things we could do, but they might be high risk and effort and low reward. Let's just brainstorm low-hanging fruit things for now...
|
|
||
Comment 6•7 years ago
|
||
(In reply to Michael Droettboom [:mdroettboom] from comment #5)
As for any changes in the code: We could move the
glean.metricsnamespace toglean.PRIVATE?
I think either glean.PRIVATE or glean.INTERNALS should be ok (all lowercase). I don't have a strong opinion on which one :)
There's perhaps more complex things we could do, but they might be high risk and effort and low reward. Let's just brainstorm low-hanging fruit things for now...
I agree with you, doesn't make much sense to go hardcore on this right now. One other thing we could do, to be more explicit, is to use annotations. We could tag the metrics with some special "DoNotUseThisIsInternalEvenIfPublic" and suppress that in glean_parsers. What do you think?
|
||
Comment 7•7 years ago
•
|
||
@RestrictTo.LIBRARY Seems to be a good candidate for annotations, but as we just learned, it does't do more than "suggest" against usage in the IDE and is not enforced by the compiler.
I'm good with the package shuffling idea, for either private or internals
|
|
||
Comment 8•7 years ago
|
||
(In reply to Travis Long from comment #7)
@RestrictTo.LIBRARYSeems to be a good candidate for annotations, but as we just learned, it does't do more than "suggest" against usage in the IDE and is not enforced by the compiler.
Good! I like this one. As long as we show some warning it's fine for me
|
|
Assignee | |
Comment 9•7 years ago
|
||
|
|
Assignee | |
Comment 10•7 years ago
|
||
|
|
Assignee | |
Comment 11•7 years ago
|
||
|
|
Assignee | |
Updated•7 years ago
|
| Comment hidden (collapsed) |
Description
•