only track/alert on installer size changes to firefox for pgo builds (what we ship)

RESOLVED FIXED in Firefox 55

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jmaher, Assigned: gps)

Tracking

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

worrying about installer size for opt builds doesn't help us much, as we ship pgo.  We don't ship pgo for osx, so opt needs to be it.

not tracking opt installer sizes could make it harder to figure out what revision caused the regression since there would be larger ranges of commits between pgo builds.

Possibly we can just turn off alerting in treeherder for opt installer size for windows and linux?  Maybe this is something we can be smart about in the code that sends perfherder_data to treeherder?
:wlach, do you have suggestions here on how to solve this?
Flags: needinfo?(wlachance)
I agree alerting for installer size on things we don't ship isn't worthwhile. Although tracking it can still be useful.

Perfherder supports defining an alerting threshold. We could crank that up for builds we don't ship so we still get data but don't alert on it.
(In reply to Gregory Szorc [:gps] from comment #2)
> I agree alerting for installer size on things we don't ship isn't
> worthwhile. Although tracking it can still be useful.
> 
> Perfherder supports defining an alerting threshold. We could crank that up
> for builds we don't ship so we still get data but don't alert on it.

That, or just disable alerting entirely by setting the shouldAlert property to false (this would be my recommendation):

https://github.com/mozilla/treeherder/blob/master/schemas/performance-artifact.json#L30

We emit the perfherder data here:

http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/testing/mozharness/mozharness/mozilla/building/buildbase.py#2004

I assume there's some kind of mozharness incantation we can do to get the appropriate information.
Flags: needinfo?(wlachance)
I'll come up with a patch.
Assignee: nobody → gps
Status: NEW → ASSIGNED
(In reply to William Lachance (:wlach) (use needinfo!) from comment #3)
> (In reply to Gregory Szorc [:gps] from comment #2)
> > I agree alerting for installer size on things we don't ship isn't
> > worthwhile. Although tracking it can still be useful.
> > 
> > Perfherder supports defining an alerting threshold. We could crank that up
> > for builds we don't ship so we still get data but don't alert on it.
> 
> That, or just disable alerting entirely by setting the shouldAlert property
> to false (this would be my recommendation):

On second thought, maybe just setting a high threshold (25%? 50%?) would be good, to catch explosions in size.
we want to catch pgo regressions that are smaller than 50%, ideally 5% or more- for opt, that seems reasonable to have a 25% threshold.
(In reply to Joel Maher ( :jmaher) from comment #6)
> we want to catch pgo regressions that are smaller than 50%, ideally 5% or
> more- for opt, that seems reasonable to have a 25% threshold.

Yeah, I wasn't proposing revisiting the alerting thresholds we decided on for platforms we ship (we decided those based on feedback from relman in bug 1353812).
Comment on attachment 8874946 [details]
Bug 1370599 - Only issue alerts on installer size changes to builds we ship;

https://reviewboard.mozilla.org/r/146322/#review150874

::: testing/mozharness/mozharness/mozilla/building/buildbase.py:1878
(Diff revision 1)
> +        if self.config.get('platform') == 'macosx64':
> +            if not self.config.get('build_variant'):
> +                return True
> +
> +        # Android opt builds are shipped.
> +        if self.config.get('platform') == 'android':

Looking at the configs, I believe all android variants will have 'android' as the platform. The sub-variants just seem to override 'stage_platform'.  Do we ship all of these android variants? If not, you may want a build_variant test similar to the OSX case.

::: testing/mozharness/mozharness/mozilla/building/buildbase.py:2026
(Diff revision 1)
> +        # We want to always collect metrics. But alerts for installer size are
> +        # only use for builds with ship. So nix the alerts for builds we don't
> +        # ship.
> +        def filter_alert(alert):
> +            if not self._is_configuration_shipped():
> +                del alert['alertThreshold']

Is this necessary given that we set shouldAlert to False?
Attachment #8874946 - Flags: review?(mshal) → review+
Comment on attachment 8874946 [details]
Bug 1370599 - Only issue alerts on installer size changes to builds we ship;

https://reviewboard.mozilla.org/r/146322/#review150874

> Looking at the configs, I believe all android variants will have 'android' as the platform. The sub-variants just seem to override 'stage_platform'.  Do we ship all of these android variants? If not, you may want a build_variant test similar to the OSX case.

Good catch. I'll change this to match OS X.

> Is this necessary given that we set shouldAlert to False?

I'm not sure. I figured that it doesn't make sense to send an alert threshold if we aren't alerting. I guess I'll retain the data in case it is still useful to Perfherder somehow.
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ccce7935260
Only issue alerts on installer size changes to builds we ship; r=mshal
https://hg.mozilla.org/mozilla-central/rev/3ccce7935260
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.