1539592 - [glean] Timespans should gracefully handle nesting of start/stop calls

Status: RESOLVED WONTFIX

(Toolkit :: Telemetry, enhancement, P3)

Description

[Michael Droettboom [:mdroettboom]]

Given the following nested calls to start/stop (which would probably be inside different nested functions where that case would be less obvious):

1 timespan.start()
2   timespan.start()
3   timespan.stopAndSum()
4 timespan.stopAndSum()

In the current implementation, line (2) will record an error, but not replace the start value. Line (3) will record the timing between (3) and (1). Line (4) will be a no-op.

It would be probably saner to detect the level of nesting in a counter and record the timing between (4) and (1). It could be argued that given that behavior, the recorded error should be demoted to a warning.

Comment 1

[Alessio Placitelli [:Dexter]]

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

Given the following nested calls to start/stop (which would probably be inside different nested functions where that case would be less obvious):

1 timespan.start()
2   timespan.start()
3   timespan.stopAndSum()
4 timespan.stopAndSum()

In the current implementation, line (2) will record an error, but not replace the start value. Line (3) will record the timing between (3) and (1). Line (4) will be a no-op.

It would be probably saner to detect the level of nesting in a counter and record the timing between (4) and (1). It could be argued that given that behavior, the recorded error should be demoted to a warning.

I'm not entirely sure we want to have such a complex behaviour. It might be more consistent with the other APIs (especially the TimespanMetricType) to just have (4) record an error rather than only no-op.

I think this needs to be discussed alongside with bug 1541493.

Comment 2

[Michael Droettboom [:mdroettboom]]

This seems like an easy fix to avoid an easy-to-find footgun.

But, I agree, using a utility class so that multiple timers could be used overlapping at the same time as suggested in 1541493 would be even better, and we could handle them in a similar manner.

Comment 3

[Michael Droettboom [:mdroettboom]]

Closing in favor of 1541493.