1346714 - Consolidate session change throttling intervals

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Georg Fritzsche [:gfritzsche]]

Currently we use two separate constants to throttle subsession changes:
- CHANGE_THROTTLE_INTERVAL_MS
- MIN_SUBSESSION_LENGTH_MS

https://dxr.mozilla.org/mozilla-central/search?q=path%3ATelemetrySession.jsm+regexp%3ACHANGE_THROTTLE_INTERVAL_MS%7CMIN_SUBSESSION_LENGTH_MS&redirect=false

We should:
- Use MIN_SUBSESSION_LENGTH_MS in both places.
- Use 5min here - the existing throttling time for the more common environment changes.
- Fix any tests.
- Make sure the documentation in sessions.rst is clear about the throttling intervals.
- Make sure preferences.rst is up-to-date.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Note that this is not a good first bug.

Comment 2

[Deepjyoti Mondal]

@Georg I would like to give this a try. So TelemetrySession.jsm is the only javascript file I need to edit right?
And where will I find preferences.rst?

Comment 3

[Deepjyoti Mondal]

@Georg should CHANGE_THROTTLE_INTERVAL_MS be removed and 5 min be hardcoded for MIN_SUBSESSION_LENGTH_MS?

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Deepjyoti Mondal from comment #3)
> @Georg should CHANGE_THROTTLE_INTERVAL_MS be removed and 5 min be hardcoded
> for MIN_SUBSESSION_LENGTH_MS?

Yes, but no hardcoding. We should just default to MIN_SUBSESSION_LENGTH_MS.

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Deepjyoti Mondal from comment #2)
> @Georg I would like to give this a try. So TelemetrySession.jsm is the only
> javascript file I need to edit right?

Yes, but you might have to change tests as well.
You can confirm tests still run fine using: mach test toolkit/components/telemetry/tests/unit
You probably want to make this test work first: mach test toolkit/components/telemetry/tests/unit/test_TelemetrySession.js

> And where will I find preferences.rst?

Try searching through the source code to find it.

Comment 6

[Deepjyoti Mondal]

@Georg thanks a lot for the pointers. I will upload a patch soon.

Comment 7

[Deepjyoti Mondal]

Attachment: bug1346714.patch

used MIN_SUBSESSION_LENGTH_MS in both places. I have tested locally, it passes all the tests

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8847664 [details] [diff] [review]
bug1346714.patch

Review of attachment 8847664 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/009fb636215b
Consolidated session change throttling intervals. r=gfritzsche

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/009fb636215b