1140132 - Annotate the current telemetry environment in crash reports

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Benjamin Smedberg]

For bug 1121013, I'd like to add the telemetry environment data as an annotation on crash reports. This will not only help to unify analysis across telemetry and crash-stats, but it will also be a way to submit a useful crash ping based on the same data.

Comment 1

[Benjamin Smedberg]

Attachment: 1140132-annotate-environment

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8581664 [details] [diff] [review]
1140132-annotate-environment

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

::: toolkit/components/telemetry/TelemetryStartup.js
@@ +21,5 @@
>  
>  TelemetryStartup.prototype.classID = Components.ID("{117b219f-92fe-4bd2-a21b-95a342a9d474}");
>  TelemetryStartup.prototype.QueryInterface = XPCOMUtils.generateQI([Components.interfaces.nsIObserver])
>  TelemetryStartup.prototype.observe = function(aSubject, aTopic, aData) {
>    if (aTopic == "profile-after-change" || aTopic == "app-startup") {

"profile-after-change" is registered for chrome processes, "app-startup" for content processes.
We probably only want to do the annotations in the chrome process?

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8581664 [details] [diff] [review]
1140132-annotate-environment

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

::: toolkit/components/telemetry/TelemetryStartup.js
@@ +35,5 @@
> +  try {
> +    let cr = Cc["@mozilla.org/toolkit/crash-reporter;1"]
> +      .getService(Ci.nsICrashReporter);
> +    let env = JSON.stringify(TelemetryEnvironment.currentEnvironment);
> +    cr.annotateCrashReport("Environment", env);

I think TelemetryEnvironment would be more descriptive here.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8602b9bc551d

Comment 5

[Benjamin Smedberg]

This ran into xperf main-thread-I/O issues.

09:00:57 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{profile}\secmod.db' was accessed and we were not expecting it. DiskReadCount: 6, DiskWriteCount: 0, DiskReadBytes: 16904, DiskWriteBytes: 0
09:00:57 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{profile}\cert8.db' was accessed and we were not expecting it. DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 33288, DiskWriteBytes: 0
09:00:57 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{profile}\key3.db' was accessed and we were not expecting it. DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 8712, DiskWriteBytes: 0 

With this c++ stack:

>	xul.dll!EnsureNSSInitialized(EnsureNSSOperator op) Line 107	C++
 	xul.dll!`anonymous namespace'::nsCryptoHashConstructor(nsISupports * aOuter, const nsID & aIID, void * * aResult) Line 199	C++
 	xul.dll!mozilla::GenericFactory::CreateInstance(nsISupports * aOuter, const nsID & aIID, void * * aResult) Line 17	C++
 	xul.dll!nsComponentManagerImpl::CreateInstance(const nsID & aClass, nsISupports * aDelegate, const nsID & aIID, void * * aResult) Line 1146	C++
 	xul.dll!nsJSCID::CreateInstance(JS::Handle<JS::Value> iidval, JSContext * cx, unsigned char optionalArgc, JS::MutableHandle<JS::Value> retval) Line 651	C++
 	xul.dll!NS_InvokeByIndex(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params) Line 71	C++

And this JS stack:

0 getIDHashForString(aStr = "Adobe AcrobatAdobe PDF Plug-In For Firefox and Nets
cape 11.0.10") ["resource://gre/modules/addons/PluginProvider.jsm":33]
1 PL_getPluginList() ["resource://gre/modules/addons/PluginProvider.jsm":198]
    this = [object Object]
2 PL_buildPluginList() ["resource://gre/modules/addons/PluginProvider.jsm":219]
    this = [object Object]
3 PL_getAddonsByTypes(aTypes = plugin, aCallback = [function]) ["resource://gre/
modules/addons/PluginProvider.jsm":144]
    this = [object Object]
4 callProviderAsync(aProvider = [object Object], aMethod = "getAddonsByTypes", a
Args = plugin,function getAddonsByTypes_concatAddons(aProviderAddons) {
"use strict";

          if (aProviderAddons) {
            addons = addons.concat(aProviderAddons);
          }
          aCaller.callNext();
        }, [function]) ["resource://gre/modules/AddonManager.jsm":235]
5 getAddonsByTypes_nextObject(aCaller = [object Object], aProvider = [object Obj
ect]) ["resource://gre/modules/AddonManager.jsm":2226]
    this = [object Object]
6 AOC_callNext() ["resource://gre/modules/AddonManager.jsm":311]
    this = [object Object]
7 getAddonsByTypes_concatAddons(aProviderAddons = [object Object],[object Object
]) ["resource://gre/modules/AddonManager.jsm":2231]
8 GMPProvider.getAddonsByTypes(aTypes = plugin, aCallback = [function]) ["resour
ce://gre/modules/addons/GMPProvider.jsm":560]
    this = [object Object]
9 callProviderAsync(aProvider = [object Object], aMethod = "getAddonsByTypes", a
Args = plugin,function getAddonsByTypes_concatAddons(aProviderAddons) {
"use strict";

          if (aProviderAddons) {
            addons = addons.concat(aProviderAddons);
          }
          aCaller.callNext();
        }, [function]) ["resource://gre/modules/AddonManager.jsm":235]
10 getAddonsByTypes_nextObject(aCaller = [object Object], aProvider = [object Ob
ject]) ["resource://gre/modules/AddonManager.jsm":2226]
    this = [object Object]
11 AOC_callNext() ["resource://gre/modules/AddonManager.jsm":311]
    this = [object Object]

I'm surprised that the crypto-hash code needs to do the full initialization of NSS, but this will clearly happen right *near* startup anyway, if the user is visiting SSL sites. Are we willing to accept this main-thread I/O hit for these files anyway, on the theory that we're going to end up doing this main-thread I/O anyway? Avi, are you the right person to make this decision?

If we do accept this, what's the right procedure to get these files added to the xperf whitelist?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for xperf regressions.
https://hg.mozilla.org/integration/fx-team/rev/dfcd9278ed70

https://treeherder.mozilla.org/logviewer.html#?job_id=2841963&repo=fx-team

Comment 7

[Avi Halachmi (:avih)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> ... 
> I'm surprised that the crypto-hash code needs to do the full initialization
> of NSS, but this will clearly happen right *near* startup anyway, if the
> user is visiting SSL sites. Are we willing to accept this main-thread I/O
> hit for these files anyway, on the theory that we're going to end up doing
> this main-thread I/O anyway? Avi, are you the right person to make this
> decision?

Typically Vladan and I _think Aaron help with the main thread IO decisions.

I'm pretty sure there wasn't and isn't a hard threshold as for what should be considered acceptable. It's a judgment call which tries to assess the overall usefulness of one approach vs another.

That being said, from what I understand so far this isn't a new thing, so it's not a "Regression", but rather a situation which needs to be assessed if it can be improved or not.

Aaron, any insights on this?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

One of the common things that we see is that something that previously caused main thread I/O outside of startup (where in the case of xperf, this means past the sessionstore-windows-restored cutoff point) is moved earlier such that it now falls under the scope of xperf.

When they can be easily avoided, we like to do so, but sometimes we just have to take the hit. NSS seems to me like an instance of the latter. I'd be ok with whitelisting these.

Comment 9

[Benjamin Smedberg]

Attachment: 1140132-nss

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8598171 [details] [diff] [review]
1140132-nss

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

Once this lands in Talos you'll also need to update the talos revision in m-c. See bug 966347.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d0f4e0591fa2
https://hg.mozilla.org/integration/fx-team/rev/40520b2ca6b8

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/dd45b61c143b
https://hg.mozilla.org/integration/fx-team/rev/9c95109b241b

Comment 13

[Carsten Book [:Tomcat]]

backed this out in case this cause also https://treeherder.mozilla.org/logviewer.html#?job_id=2868187&repo=fx-team

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1815927ac2fc

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1815927ac2fc