matomo-org/matomo-sdk-android

Device information only report for first session per process

kevinslashslash opened this issue · 3 comments

Device information, such as resolution, user agent and language, only get reported for the first session causing "unknown" screen resolution and "Unknown" device brands and "Generic Mobile device" device models.
Steps to reproduce:
Setup backend and Demo App
In Demo App press Track Screen View then Dispatch Now
Verify this is reporting in Visits Log or Real-time. See that the device info is set (specifically resolution as brand sometimes makes it through).
Now without having exited the Demo App, press Track Screen View and then Dispatch Now again.
This time check the reporting and the resolution will say "unknown"

This is likely related to this code:

if (mLastEvent == null || !Objects.equals(trackMe.get(QueryParams.USER_ID), mLastEvent.get(QueryParams.USER_ID))) {

mLastEvent being non-null is being used as a hint that this is part of an existing session, but that isn't necessarily true. In the case of the demo app, Track Screen View is specifically starting a new session, but it could also happen by timeout.
I think it'd be most appropriate to move setting of three parameters (SCREEN_RESOLUTION, USER_AGENT, LANGUAGE) into their own method and call it from track if SESSION_START is set.
Alternatively if the way "Track Screen View" is starting a new session just by setting SESSION_START is inappropriate, that code should be changed and the three parameters could be part of injectInitialParams instead.

What do you think about extending the newSession check, to also check for the new session flag in the TrackMe:

final boolean newSession = System.currentTimeMillis() - mSessionStartTime > mSessionTimeout;
if (newSession) {
mSessionStartTime = System.currentTimeMillis();
injectInitialParams(trackMe);
}

I think it'd be most appropriate to move setting of three parameters (SCREEN_RESOLUTION, USER_AGENT, LANGUAGE) into their own method and call it from track if SESSION_START is set.

Maybe we simplify this and also call it for every inject base parameters call, starting to seem like this optimization is more trouble than it's worth... 🤔

The fix in my pull request does fix the issue as described, however it doesn't eliminate sessions with no device information.

I think another way this can happen is that the session start event gets purged from the cache but the rest of the session doesn't. This could be addressed either by injecting the parameters to the session at dispatch time, which seems wrong as it's mixing responsibilities, injecting on every event, or altering dispatch logic to not even submit sessions when the session start was dropped.

Personally I think I'm now in favor of injecting on every event.

I'm also leaning towards using the simpler solution, just inject on every event. The small gain is not worth the bag of edge cases.