1596442 - Provide crash resistance to 'ping' lifetime metrics when they are not stored to disk

Status: RESOLVED FIXED

(Data Platform and Tools :: Glean: SDK, enhancement, P1)

Description

[Alessio Placitelli [:Dexter]]

This is a follow-up to bug 1596440.

As a stretch goal, we could provide crash safety by persisting ping-lifetime metrics to disk every N minutes.

Comment 1

[Beatriz Rizental [:brizental]]

I think it is time to start thinking about how to implement this.

I understand the concept of persisting every N minutes (or some other time measurement), but what is not clear to me is how we define this interval.

One way is to make this a configurable thing, and change the newly added defer_ping_lifetime_io to ping_lifetime_io_interval or something of the sort. I don't think I like this solution, because it is such an internal thing to glean that it will probably not be very intuitive for a user of the API, which can lead to misconfiguration or a more frustrating setup for them.

Another way would be to set a default interval, but then, how do we decide how long is that interval?

I remember [:janerik] talking about making the whole deferring of ping lifetime data a build option, so there is also that way to go.

I am probably missing other possibilities here. So, please give me your input :)

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Beatriz Rizental from comment #1)

One way is to make this a configurable thing, and change the newly added defer_ping_lifetime_io to ping_lifetime_io_interval or something of the sort. I don't think I like this solution, because it is such an internal thing to glean that it will probably not be very intuitive for a user of the API, which can lead to misconfiguration or a more frustrating setup for them.

Another way would be to set a default interval, but then, how do we decide how long is that interval?

Let's start by not making this configurable but, rather, provide a default. I think we can safely start with the default from the Desktop aborted-session ping.

Chris, would persisting 'ping lifetime' metrics to disk every 15 minutes (and, potentially, shutdown) be enough?
The 15 minutes mark was picked up, back then, by looking at the session length and hitting the 99 or 95 percentile mark.

I remember [:janerik] talking about making the whole deferring of ping lifetime data a build option, so there is also that way to go.

What do you mean here? :)

Comment 3

[Beatriz Rizental [:brizental]]

(In reply to Alessio Placitelli [:Dexter] from comment #2)

Let's start by not making this configurable but, rather, provide a default. I think we can safely start with the default from the Desktop aborted-session ping.

Awesome, sounds good. I think it is important to persist on shutdown too, so that we don't lose any data if the user shuts down in between the 15min windows.

What do you mean here? :)

Oh, I think the best person to answer that would be [:janerik]. I just remember him mentioning something of the sort and added that to my comment so that it could possibly be discussed.

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Beatriz Rizental from comment #3)

(In reply to Alessio Placitelli [:Dexter] from comment #2)

Let's start by not making this configurable but, rather, provide a default. I think we can safely start with the default from the Desktop aborted-session ping.

Awesome, sounds good. I think it is important to persist on shutdown too, so that we don't lose any data if the user shuts down in between the 15min windows.

Sure :) Just for completeness, note that we might not be able to detect 'shutdown' reliably ;)

Comment 5

[Jan-Erik Rediger [:janerik]]

I'm with :dexter on this: Pick a reasonable value for now and hard-code it.
I'm not sure anymore in what context I mentioned the build-time option.
The only thing I can think of right now is having this as a cargo feature. But for now I wouldn't go that way.

One thing to think about is whether the timer is in glean-core itself or on the platform layer.
I currently imagine the persistence just being an exposed function and the platform layer/application is responsible for calling it in an interval (the reason for this: we might want to use Firefox/Gecko's own thread model/timer hooks to run it)

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #5)

I currently imagine the persistence just being an exposed function and the platform layer/application is responsible for calling it in an interval (the reason for this: we might want to use Firefox/Gecko's own thread model/timer hooks to run it)

If we go with this, then we don't even need to hardcode a timer value, we only need to expose a function to persist ping lifetime data.

Chris is the best person to ask what's the model he prefer, but I suspect giving more flexibility to Gecko is the way to go., as Jan-Erik suggested.

Comment 7

[Chris H-C :chutten|PTO (back July 13)]

First and foremost, Alessio, I think you mean 5 minutes, not 15? (At least that's what's at the link you provided)

As for control mechanism (the choice between it being a mode I turn on and an API I can call periodically)... Hm. We already need a mode to disable the "persist every single action" mode that the SDK defaults to, which makes me lean towards it being a member of Configuration (e.g. ping_lifetime_persistence_interval_minutes with a default of 0 for mobile clients). It has the benefit of placing the implementation firmly in the platform-independent layer at the cost of not giving the application the control to decide whether or not this 5min is a good one or not (maybe we're asleep and this will be waking us up? Maybe we should dispatch until we're idle with a max 5min timer?).

In other words, I'm torn.

I think that, since it'll be easier to code and test, we should go for the API model for now and force the embedding application to decide how they'd like to control it.

Comment 8

[Beatriz Rizental [:brizental]]

Attachment: GitHub Pull Request