1330179 - [AS] [Telemetry] Telemetry session is not being set while we're on the AS panel

Status: RESOLVED DUPLICATE of bug 1331059

(Firefox for Android Graveyard :: General, defect, P1)

Description

[:Grisha Kruglov]

Looking at the data in redash, we can see that individual probes are firing as expected and rich AS data is in place, but somehow we're not setting the session name.

Here's the debug query: https://sql.telemetry.mozilla.org/queries/2083/source

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

I had problems with finding sessions in re:dash in an other bug too. Maybe there's a problem with sessions in general.

Comment 2

[:Grisha Kruglov]

Well, on a surface it would appear that session are working, somewhat.

And perhaps the most interesting, here is a breakdown of which sessions actually make it along with the probes:
https://sql.telemetry.mozilla.org/queries/2094/source

~0.2%: ["reader"]
~0.017%: ["awesomescreen","reader"]
~8%: ["awesomescreen"]
~0.2%: ["awesomescreen","firstrun"]
~90%: []
~0%: ["awesomescreen","firstrun","reader"]
~0%: ["firstrun","reader"]
~1.8%: ["firstrun"]

So, most probes that fire don't have a session set at all, and some are clearly incorrect. E.g. some A-S probes fire with a session "awesomescreen".

Comment 3

[:Grisha Kruglov]

Comment 2 shows nightly data (and is missing a paragraph, it seems). Release and beta have similar breakdowns - no session is about 90-92%, "awesomescreen" by itself is about 7-8%. Note that sessions such as "settings", "frecency", "homepanel:*", "search_activity" don't make an appearance at all, even though we start/stop them in the code.

Comment 4

[:Grisha Kruglov]

Unless I'm missing something, looking through UITelemetry.jsm and friends (TelemetryController, TelemetrySend, etc), we pretty much send off ping data as is, with session data intact. So it seems like there might be something else at play here? Our sessions are named something like "awesomescreen.1", and I think that is what we send off in a telemetry ping. And yet ".1" gets stripped off the session name at some point.

Comment 5

[:Grisha Kruglov]

A workaround which should unblock building dashboards/queries for A-S data is to filter for it using something like: ... WHERE extras like "%fx_account_present%" ....

This relies on the fact that every A-S telemetry ping will have this extras field present, and no other telemetry is currently using anything like this.

Queries using this hack will be easily switchable to a proper sessions-based way of filtering once that's working.

Comment 6

[:Grisha Kruglov]

Georg, I wonder if you might help shed some light here. As far as I can tell, we're sending off telemetry pings with session data intact from the devices, but somewhere along the way "sessions" part of the payload is getting lost. It's not every payload that's affected - some of the sessions are unaffected and show up in redash (see Comment 2). One thing that seems to be in common (although I haven't verified it yet) is that the "lost" session names have a form of "activitystream.1:newtab", and the ones that make it look like "awesomebar.1", i.e. there's no colon-separated suffix. Could there be some server-side processing involved that's filtering certain sessions out?

Comment 7

[Georg Fritzsche [:gfritzsche]]

Frank, this sounds like this could be an ETL issue with the mobile job for android_events_v1?

Comment 8

[Frank Bertsch [:frank]]

Yes, this is a construct of the android_events ETL job. I'm not totally sure what the purpose was, but you can see[0] that the code is basically doing (note this is cleaned up a bit):

> for session in event["sessions"]:
>   if "experiment.1:" in session:
>     experiments.append(safe_str(session[13:]))
>   elif "firstrun.1" in session:
>     sessions.add("firstrun")
>   elif "awesomescreen.1" in session:
>     sessions.add("awesomescreen")
>   elif "reader.1" in session:
>     sessions.add("reader")

We can absolutely change it - I'm going to be doing some data engineering work on mobile stuff this Q, so we can add in stuff like this no problem.

[0] https://github.com/mozilla-services/data-pipeline/blob/master/reports/android-events/android-events.ipynb

Comment 9

[Georg Fritzsche [:gfritzsche]]

There are notes on sessions and the versioning here:
https://gecko.readthedocs.io/en/latest/mobile/android/fennec/uitelemetry.html

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, this explains why I couldn't find any 'search_activity' sessions too. I guess it won't be possible (or too 'expensive') to recover the sessions for past events?

Comment 11

[Frank Bertsch [:frank]]

(In reply to Sebastian Kaspari (:sebastian) from comment #10)
> Oh, this explains why I couldn't find any 'search_activity' sessions too. I
> guess it won't be possible (or too 'expensive') to recover the sessions for
> past events?

For one-off analysis, we could do something with a subset of historical data and it would still be statistically accurate. Backfill of these changes for all data would not be something I'd recommend.

Comment 12

[:Grisha Kruglov]

(In reply to Frank Bertsch [:frank] from comment #8)
> Yes, this is a construct of the android_events ETL job. I'm not totally sure
> what the purpose was, but you can see[0] that the code is basically doing
> (note this is cleaned up a bit):
> 
> > for session in event["sessions"]:
> >   if "experiment.1:" in session:
> >     experiments.append(safe_str(session[13:]))
> >   elif "firstrun.1" in session:
> >     sessions.add("firstrun")
> >   elif "awesomescreen.1" in session:
> >     sessions.add("awesomescreen")
> >   elif "reader.1" in session:
> >     sessions.add("reader")
> 
> We can absolutely change it - I'm going to be doing some data engineering
> work on mobile stuff this Q, so we can add in stuff like this no problem.
> 
> [0]
> https://github.com/mozilla-services/data-pipeline/blob/master/reports/
> android-events/android-events.ipynb

The client code which generates this telemetry makes an assumption that a session name will make it intact as it is constructed. We have a version number, allowing us to slice sets of events accordingly (granted, all of the sessions are currently at v1). We also have a concept of a session suffix. E.g. an event might have a session name such as "homepanel.1:1234-1234-1234", specifying an ID of the specific home panel which is active for the event, helping us to further narrow down scope of events.

This processing code doesn't seem to be in agreement. It would be good to understand why it's there, and perhaps the reason is irrelevant now, and we can let our session names remain intact going forward? This will let us add new session names without a need to make changes to the pipeline. We'll need to be careful and adjust any queries which currently depend on this processing to happen.

Comment 13

[Frank Bertsch [:frank]]

(In reply to :Grisha Kruglov from comment #12)
> (In reply to Frank Bertsch [:frank] from comment #8)
> The client code which generates this telemetry makes an assumption that a
> session name will make it intact as it is constructed. We have a version
> number, allowing us to slice sets of events accordingly (granted, all of the
> sessions are currently at v1). We also have a concept of a session suffix.
> E.g. an event might have a session name such as
> "homepanel.1:1234-1234-1234", specifying an ID of the specific home panel
> which is active for the event, helping us to further narrow down scope of
> events.
> 
> This processing code doesn't seem to be in agreement. It would be good to
> understand why it's there, and perhaps the reason is irrelevant now, and we
> can let our session names remain intact going forward? This will let us add
> new session names without a need to make changes to the pipeline. We'll need
> to be careful and adjust any queries which currently depend on this
> processing to happen.

Grisha, I made Bug 1331059 to track these changes.

Comment 14

[:Grisha Kruglov]

I'm going to close this bug in lieu of the data pipeline bug, since that's where the changes are going to land.