Closed annthurium closed 6 years ago
@jasonrudolph, I believe I have addressed all your feedback and am ready to merge. Thanks again for your review!
I believe I have addressed all your feedback and am ready to merge.
@annthurium: Thanks for the ping.
Following up on https://github.com/atom/telemetry/pull/12#discussion_r195159199, I'd still like to recommend that we don't think of "custom events" as "custom." Quite the contrary: I think this should be the default/recommended way of recording events. These events provide richer information (by automatically including the timestamp of the event and supporting arbitrary metadata), and they're consistent with the event reporting approach in commonly-used tools like Google Analytics and Datadog.
To me, the simple usage counts are a more custom concept for a metrics library to provide. Along those lines, if we wanted to do so, I think we could implement the simple usage counts in terms of the more generic events implemented in this PR. For example, for events that should be aggregated locally and reported to the metrics backend on a daily basis:
store.increment('some-count-to-be-aggregated', {aggregate: true})
Or, perhaps we could let clients specify which events should be aggregated when initializing the client:
store = new StatsStore("atom", "1.24.1", {aggregatedEvents: [
'some-count-to-be-aggregated',
'some-other-count-to-be-aggregated',
]})
store.increment('some-count-to-be-aggregated') // will be aggregated
store.increment('file-open', {grammar: 'source.js'}) // won't be aggregated
store.increment('file-rename') // won't be aggregated
store.increment('some-other-count-to-be-aggregated') // will be aggregated
Would you consider changing the terminology of "custom events" in the code and in the docs to remove the "custom" phrasing and refer to them as the primary way that clients should record event data?
I don't think "custom" implies "not the default." I think it implies "flexibility to send whatever data you want." I'm going to run these names past a few other devs and see how they interpret it and then get back to you - that way we have a few more data points about the clarity (or lack thereof) of this terminology. :-) thanks for bringing it up!
There are some event types that are common across all client apps.
usage
events,ping
events, andopt in / out
events. Telemetry encapsulates the complexity around those so clients don't have to care.However, apps might want to collect more flexible data. For example, Atom currently collects "file open" events, which preserve the grammar (e.g. language) of the opened file. That way we can answer questions like "what languages are most popular among users of Atom? (More on that in https://github.com/atom/telemetry/issues/8#issuecomment-396319097).
This pull request adds the ability for
telemetry
to support custom/arbitrary event data that the clients pass in, which is then passed along to Central with the daily metrics dump.