adding best practices for telemetry events design#197
adding best practices for telemetry events design#197chutten merged 6 commits intomozilla:masterfrom
Conversation
chutten
left a comment
chutten
left a comment
There was a problem hiding this comment.
It isn't clear from the document the problem we're trying to solve with this. I recommend providing an example (perhaps one from your own experience) showing the difficulty in finding the definition and code when all you have are rows from the events dataset or user flows in Amplitude.
For instance, trying to nail down where devtools.main.open comes from in the code is tricky. This is because each of the category, method, and object are too common in the code base and in Firefox data collections.
src/SUMMARY.md
Outdated
| * [Metrics](cookbooks/metrics.md) | ||
| * [Active DAU Definition](cookbooks/active_dau.md) | ||
| * [Retention Analysis](cookbooks/retention.md) | ||
| * [Designing Telemetry Events Best Practices](cookbooks/designing_events_best_practices.md) |
There was a problem hiding this comment.
I think we could drop "Designing" here and gain brevity without losing meaning.
|
|
||
| ## Naming Events/Probes: | ||
|
|
||
| One of the limitations of the Telemetry Events API is that it is difficult to distinguish between events produced by different probes. The API allows for a lot of flexibility. Any probe written into Firefox can create an event with any values the following fields: |
There was a problem hiding this comment.
The wording here I think should be "can record an event" or "can create an event record".
Also, somewhere around here you can link out to this part of the docs for the technical docs.
| * The Category, Method, and Object of any event produced by the probe must have a value. | ||
| * All combinations of Category, Method, and Object values must be unqiue to that probe (no other probes can produce the same combination). | ||
|
|
||
| However, there is no identifier or name field for the probe itself. Furthermore, although the combinations of Category, Method, and Object for a probe must be unique, individually, those fields can share values with other probes. |
There was a problem hiding this comment.
I'd avoid calling attention to the "name field" as there is indeed an event name field: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/events.html#the-yaml-definition-file
You can instead contrast this to Histograms and Scalars whose definitions each have a unique identifier: their metric name.
|
|
||
| However, there is no identifier or name field for the probe itself. Furthermore, although the combinations of Category, Method, and Object for a probe must be unique, individually, those fields can share values with other probes. | ||
|
|
||
| This can make it confusing and difficult to isolate events from a particular probe, or find what probe produced a specific event. |
There was a problem hiding this comment.
The use of the word "probe" is unclear here. What is meant by it?
|
@chutten , good points. I've updated the cookbook to provide more background and give some more concrete examples to make the need more clear. Let me know what you think. |
chutten
left a comment
chutten
left a comment
There was a problem hiding this comment.
I apologize for leaving this for so long.
|
|
||
| With the following restrictions and features: | ||
| * The category, method, and object properties of any record produced by an event must have a value. | ||
| * All combinations values from the category, method, and object properties must be unique to that particular event (no other event can produce events with the same combination). |
There was a problem hiding this comment.
| * All combinations values from the category, method, and object properties must be unique to that particular event (no other event can produce events with the same combination). | |
| * All combinations of values from the category, method, and object properties must be unique to that particular event (no other event can be registered with the same combination). |
| * All combinations values from the category, method, and object properties must be unique to that particular event (no other event can produce events with the same combination). | ||
| * Events can be 'turned on' or 'turned off' by it's category value. i.e. we can instruct the browser to "stop sending us events from 'devtools' category." | ||
|
|
||
| These records are then stored in event pings and available in the events dataset. |
There was a problem hiding this comment.
I recommend linking out to the event pipeline and dataset docs here.
|
|
||
| which can further confuser users. | ||
|
|
||
| [1]: If a user defines an event in Events.yaml without specifying a list of acceptable methods, the method will default to the name of the event for records created by that event. |
There was a problem hiding this comment.
You may need to preface this footnote with "events do have name fields, but they aren't included in the event records and thus are not present in the resulting dataset. Also, If a user defines..."
| "category.event_name" | ||
| ``` | ||
|
|
||
| For example: |
There was a problem hiding this comment.
Could you expand the example? Maybe take the opportunity to show how the addonsManager.manage example definition above would be changed by this convention.
|
|
||
|
|
||
| This provides 3 advantages: | ||
| 1. Records produced by this event will be easily identifiable. Conversely, the event which produced the record will be easier to locate in the code. |
There was a problem hiding this comment.
"Conversely" is usually reserved for statements that oppose the previous one. I'd go with "Also" here.
|
|
||
| This provides 3 advantages: | ||
| 1. Records produced by this event will be easily identifiable. Conversely, the event which produced the record will be easier to locate in the code. | ||
| 2. Events can be controlled easier. The category field is what we use to "turn on" and "turn off" events. By creating a 1 to 1 mapping between categories and events, we can control events on an individual level. |
There was a problem hiding this comment.
| 2. Events can be controlled easier. The category field is what we use to "turn on" and "turn off" events. By creating a 1 to 1 mapping between categories and events, we can control events on an individual level. | |
| 2. Events can be controlled more easily. The category field is what we use to "turn on" and "turn off" events. By creating a 1 to 1 mapping between categories and events, we can control events on an individual level. |
|
@SuYoungHong @chutten Future improvements aside, are there any blocking changes needed here? Could the rest be moved to follow-ups? |
updating doc r.e. suggestions from chutten
|
looks like it's also running into issues with travis. It's failing tests based on spelling for the following words (filenames and event field values):
how do you advise I resolve these? should I treat event field values as code in |
|
Code blocks might be the most sensible way. The alternative is to add it to the list of exceptions in |
|
@SuYoungHong : I got a fantastic tip from @tdsmith to install the mdspell, so that you can run it locally before submitting the PR and fix all the spelling errors before it goes through CI:
|
fixed spelling nits (into code) for travis tests
|
thanks @ethompso28 ! I'll check it out! |
|
cool, tests pass now. PR ready to merge if @chutten is okay with pushing the final change request to another PR (if not, let me know and I can make the change in this PR)
|
|
I'm fine with it happening as a follow-up. Thanks for your persistence in this! |
4 participants
Added cookbook for telemetry events design best practices. RE this bug
Not sure where in the docs this should live, so put it into cookbooks by default.