Closed Bug 1109931 Opened 5 years ago Closed 5 years ago

Save child telemetry payload on shutdown

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(1 file, 1 obsolete file)

Once telemetry is running and we have shutdown notification in the child process, we should send over telemetry data to the parent on shutdown, so the child payloads can be saved inside the parent payload.
I took some liberty in taking things that I think don't apply to the child out of the child payload. In summary, when we shut down in the child, we take the payload and send it over to the parent. The parent makes sure that it only has one saved payload per child, and eventually puts the child payloads in the parent payload.
Attachment #8534657 - Flags: review?(vdjeric)
Comment on attachment 8534657 [details] [diff] [review]
Save child telemetry payload on shutdown (v1)

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +19,5 @@
> +const CONTENT_PROCESS = (function() {
> +  let runtime = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);
> +  return runtime.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT;
> +})();
> +

how about:
const IS_CONTENT_PROCESS = (Services.appinfo.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT);

@@ +501,5 @@
>     */
>    getMetadata: function getMetadata(reason) {
> +    if (CONTENT_PROCESS) {
> +      return null;
> +    }

I would prefer if we just never called getMetadata() in the child process

@@ +765,5 @@
>        threadHangStats: this.getThreadHangStats(Telemetry.threadHangStats),
> +      lateWrites: !CONTENT_PROCESS ? Telemetry.lateWrites : null,
> +      addonHistograms: !CONTENT_PROCESS ? this.getAddonHistograms() : null,
> +      addonDetails: !CONTENT_PROCESS ? AddonManagerPrivate.getTelemetryDetails() : null,
> +      UIMeasurements: !CONTENT_PROCESS ? UITelemetry.getUIMeasurements() : null,

This is a bit ugly and we end up sending empty fields that aren't applicable to the child process.

Could we just define payloadObj with all the common fields at the beginning of assemblePayloadWithMeasurements(), and then just "if (CONTENT_PROCESS) { return payloadObj; }"

The child payloads don't need the clientID field btw

@@ +1127,5 @@
> +      });
> +      break;
> +    }
> +    default:
> +      throw new Error("Bad message name");

Would this throw get caught or reported anywhere?

@@ +1131,5 @@
> +      throw new Error("Bad message name");
> +    }
> +  },
> +
> +  saveContentProcessPing: function saveContentProcessPing(reason) {

how about sendContentProcessPing? Because we're going to re-use this method for idle-daily Telemetry

@@ +1134,5 @@
> +
> +  saveContentProcessPing: function saveContentProcessPing(reason) {
> +    let payload = this.getSessionPayload(reason);
> +    // Send a sync message to make sure it arrives before we shut down IPC.
> +    cpmm.sendAsyncMessage(MESSAGE_TELEMETRY_PAYLOAD, payload);

shouldn't this be sendSyncMessage?
Attachment #8534657 - Flags: review?(vdjeric)
(In reply to Vladan Djeric (:vladan) from comment #2)
> Comment on attachment 8534657 [details] [diff] [review]
> Save child telemetry payload on shutdown (v1)
> 
> ::: toolkit/components/telemetry/TelemetryPing.jsm
> @@ +19,5 @@
> > +const CONTENT_PROCESS = (function() {
> > +  let runtime = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);
> > +  return runtime.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT;
> > +})();
> > +
> 
> how about:
> const IS_CONTENT_PROCESS = (Services.appinfo.processType ==
> Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT);

Just tried it but it didn't sit well with the telemetry xpcshell tests. The problem was, in xpcshell, we don't really have Services.appinfo until the test specifically creates its own appinfo object, so we can't really access Services.appinfo when inside a test at the global scope. There are ways around it but I think my original approach is the best one for now. I can file a follow-up bug for this.

> @@ +501,5 @@
> >     */
> >    getMetadata: function getMetadata(reason) {
> > +    if (CONTENT_PROCESS) {
> > +      return null;
> > +    }
> 
> I would prefer if we just never called getMetadata() in the child process

Fixed.

> @@ +765,5 @@
> >        threadHangStats: this.getThreadHangStats(Telemetry.threadHangStats),
> > +      lateWrites: !CONTENT_PROCESS ? Telemetry.lateWrites : null,
> > +      addonHistograms: !CONTENT_PROCESS ? this.getAddonHistograms() : null,
> > +      addonDetails: !CONTENT_PROCESS ? AddonManagerPrivate.getTelemetryDetails() : null,
> > +      UIMeasurements: !CONTENT_PROCESS ? UITelemetry.getUIMeasurements() : null,
> 
> This is a bit ugly and we end up sending empty fields that aren't applicable
> to the child process.
>
> Could we just define payloadObj with all the common fields at the beginning
> of assemblePayloadWithMeasurements(), and then just "if (CONTENT_PROCESS) {
> return payloadObj; }"
> 
> The child payloads don't need the clientID field btw

Fixed.

> @@ +1127,5 @@
> > +      });
> > +      break;
> > +    }
> > +    default:
> > +      throw new Error("Bad message name");
> 
> Would this throw get caught or reported anywhere?

It should be reported in the browser console.

> @@ +1131,5 @@
> > +      throw new Error("Bad message name");
> > +    }
> > +  },
> > +
> > +  saveContentProcessPing: function saveContentProcessPing(reason) {
> 
> how about sendContentProcessPing? Because we're going to re-use this method
> for idle-daily Telemetry

Changed.

> @@ +1134,5 @@
> > +
> > +  saveContentProcessPing: function saveContentProcessPing(reason) {
> > +    let payload = this.getSessionPayload(reason);
> > +    // Send a sync message to make sure it arrives before we shut down IPC.
> > +    cpmm.sendAsyncMessage(MESSAGE_TELEMETRY_PAYLOAD, payload);
> 
> shouldn't this be sendSyncMessage?

We needed sync in an earlier version, but now async is fine and probably has better performance. I removed the comment.
Attachment #8534657 - Attachment is obsolete: true
Attachment #8537492 - Flags: review?(vdjeric)
Comment on attachment 8537492 [details] [diff] [review]
Save child telemetry payload on shutdown (v2)

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

(In reply to Jim Chen [:jchen] from comment #3)
> Just tried it but it didn't sit well with the telemetry xpcshell tests. The
> problem was, in xpcshell, we don't really have Services.appinfo until the
> test specifically creates its own appinfo object, so we can't really access
> Services.appinfo when inside a test at the global scope. There are ways
> around it but I think my original approach is the best one for now. I can
> file a follow-up bug for this.

Yes, please file a bug. This patch can land without that improvement

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +67,5 @@
>                                     "@mozilla.org/widget/idleservice;1",
>                                     "nsIIdleService");
> +XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
> +                                   "@mozilla.org/childprocessmessagemanager;1",
> +                                   "nsISyncMessageSender");

shouldn't the interface be nsIMessageSender since the child isn't doing sync sends?

@@ +334,5 @@
> +    ret.js = Cu.getJSEngineTelemetryValue();
> +
> +    let maximalNumberOfConcurrentThreads = Telemetry.maximalNumberOfConcurrentThreads;
> +    if (maximalNumberOfConcurrentThreads)
> +      ret.maximalNumberOfConcurrentThreads = maximalNumberOfConcurrentThreads;

let's add some curlies around the if-statement :)

@@ +1129,5 @@
> +      });
> +      break;
> +    }
> +    default:
> +      throw new Error("Bad message name");

Let's make the error more descriptive so it's easier to track down: "TelemetryPing.receiveMessage: bad message name"
Attachment #8537492 - Flags: review?(vdjeric) → review+
https://hg.mozilla.org/mozilla-central/rev/d8e1bdfff9f5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.