1597747 - TimingDistribution.set_start doesn't check if upload is enabled

Status: RESOLVED FIXED

(Data Platform and Tools :: Glean: SDK, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

When calling TimingDistribution.start we don't check if upload is enabled. We just check that in set_stop_and_accumulate which means that, if upload is disabled when the recording is started, we leave 'TimerId's hanging.

Note: this is not too much of a big deal, since nothing really gets persisted if upload is off, given the check in set_stop_and_accumulate

Comment 1

[Michael Droettboom [:mdroettboom]]

When calling TimingDistribution.start we don't check if upload is enabled. We just check that in set_stop_and_accumulate which means that, if upload is disabled when the recording is started, we leave 'TimerId's hanging.

Looking closely at the code, stop_and_accumulate does actually handle the timer id and clear it before checking should_record. I think this is the correct behavior, given that we want to implicitly handle timer ids always so we always get correct time measurements regardless of flipping the set_upload_enabled bit.

I'd be in favor of marking this as FIXED. What do you think, :Dexter?

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Michael Droettboom [:mdroettboom] from comment #1)

Looking closely at the code, stop_and_accumulate does actually handle the timer id and clear it before checking should_record. I think this is the correct behavior, given that we want to implicitly handle timer ids always so we always get correct time measurements regardless of flipping the set_upload_enabled bit.

This would still allow us to start a timer, then flip telemetry off, turn it on again, and it would accumulate a time that spans across the toggling.

Looking at this, it seems like we only really have 2 options:

1. we check for "upload enabled" when calling start, and fail returning a placeholder id/error
2. we keep the current behaviour and clear all the data associated with the existing metric handles, when toggling this on or off.

From a quick look, (1) looks more reasonable.

I'd be in favor of marking this as FIXED. What do you think, :Dexter?

If we're willing to accept that we act weird when toggling the upload on/off multiple time in a single session, then we can close this as WONTFIX: I still think we have an odd behaviour, unless I'm getting this wrong (and I can be!).

Comment 3

[Michael Droettboom [:mdroettboom]]

My only concern is that when upload is disabled we (1) don't leak timer handles and (2) don't report data. (Which is the case presently). But I don't think it's the right thing to break up timings based on upload toggling. The timing is independent of that, so I really think current behavior is correct. But maybe we bring some others into the discussion to help break the tie?

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Michael Droettboom [:mdroettboom] from comment #3)

But maybe we bring some others into the discussion to help break the tie?

As you prefer! However, as stated in comment 2, I think we can close it.

I think we just disagree on whether or not the fact that we can record the value for a timer when upload is toggled twice within a session is odd or not. I personally don't feel strongly about this, as we currently behave the same on Desktop.