Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New telemetry ping for Pocket + FireTV events #1606

Closed
psymoon opened this issue Dec 20, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@psymoon
Copy link
Contributor

commented Dec 20, 2018

Need client implementation to support the new ping added here

@pocmo pocmo added the <telemetry> label Dec 21, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

@psymoon Thank you for filing. Is there a Fire TV issue for that too? I'm curious what the priority is (cc @athomasmoz).

@Sdaswani In a mail thread you mentioned that you are interested in starting to use glean in FFTV in Q1. Do we still need this new ping in the old telemetry library?

@Sdaswani

This comment has been minimized.

Copy link

commented Dec 21, 2018

@pocmo I'm indeed interested but haven't made any firm plans. It may not happen until late Q1, or maybe pushed to Q2. In any case we wouldn't be using it for production data analysis given the pipeline won't be signed off for use until Q3.

@psymoon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Alright, sounds like we need this in service-telemetry then. I added the a-c label to the FFTV issue. Since the AC team has a lot on its plate I wonder if the FFTV team would like to pick this up? I can provide some hints what needs to be done here.

@psymoon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@pocmo yes, I would gladly work on this!

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Great!

To add a new ping type we need to implement a new TelemetryPingBuilder.

An example is the mobile-event ping. You posted a schema for your ping above. This is the schema for the mobile-event ping:
https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/dc458113a7a523e60a9ba50e1174a3b1e0cfdc24/schemas/telemetry/mobile-event/mobile-event.1.schema.json

And implemented in the telemetry this is here in TelemetryMobileEventPingBuilder:
https://github.com/mozilla-mobile/android-components/blob/master/components/service/telemetry/src/main/java/org/mozilla/telemetry/ping/TelemetryMobileEventPingBuilder.java

Following this pattern you'll need to implement some kind of TelemetryFireTVEventPingBuilder class where you translate the fields in the schema to "measurements" in the ping builder. You may find some of them already implemented here (e.g. "os" or "osversion"):
https://github.com/mozilla-mobile/android-components/tree/master/components/service/telemetry/src/main/java/org/mozilla/telemetry/measurement

Add new ones as you need them.

Note that the base class (TelemetryPingBuilder) already adds the version and client id:
https://github.com/mozilla-mobile/android-components/blob/master/components/service/telemetry/src/main/java/org/mozilla/telemetry/ping/TelemetryPingBuilder.java

Your schema seems to deliberately not send the client id. That's a special case we didn't have before, so you'll need to workaround that.

In Fire TV you'll find code that adds the ping builders to the telemetry library and schedules pings. That's something you'd need to add after the release. However make sure your ping builder exposes access to measurements where the app needs to set values (unlike "os" where the measurement can figure out the value on its own). For example the core ping builder exposes some measurements here that the app will set itself: https://github.com/mozilla-mobile/android-components/blob/master/components/service/telemetry/src/main/java/org/mozilla/telemetry/ping/TelemetryCorePingBuilder.java#L60-L78

Most measurement implementations fall in one of those three categories:

  • static: The value is fixed or can be obtained from the system (OS, OS version, Dates..)
  • in memory: The value is set by the app and kept in the measurement until needed: (list of experiments, default search engine, ..)
  • persisted: The value(s) are saved to disk because we want to accumulate multiple values or need them for later pings too (search counts, events, ..)

For testing:

  • The library has a high coverage and those things are all nicely testable in unit tests
  • When integrating: You can use the DebugLogClient to log pings to logcat instead of uploading them

The telemetry library was written in Java before we started to use Kotlin. However feel free to use Kotlin for new code. I'd suggest not migrating any existing code at this time though (because of risk and since the library will go away eventually anyways) - unless really needed.

Ping me if you need any help!

psymoon added a commit to psymoon/android-components that referenced this issue Jan 6, 2019

psymoon added a commit to psymoon/android-components that referenced this issue Jan 6, 2019

psymoon added a commit to psymoon/android-components that referenced this issue Jan 7, 2019

psymoon added a commit to psymoon/android-components that referenced this issue Jan 7, 2019

psymoon added a commit to psymoon/android-components that referenced this issue Jan 7, 2019

psymoon added a commit to psymoon/android-components that referenced this issue Jan 7, 2019

psymoon added a commit to psymoon/android-components that referenced this issue Jan 7, 2019

psymoon added a commit to psymoon/android-components that referenced this issue Jan 7, 2019

psymoon added a commit to psymoon/android-components that referenced this issue Jan 7, 2019

psymoon added a commit to psymoon/android-components that referenced this issue Jan 7, 2019

pocmo added a commit that referenced this issue Jan 8, 2019

pocmo added a commit that referenced this issue Jan 8, 2019

pocmo added a commit that referenced this issue Jan 8, 2019

@pocmo pocmo closed this in 74ea893 Jan 8, 2019

pocmo added a commit that referenced this issue Jan 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.