Add ReportingObserver #47

RByers · 2017-09-01T19:53:41Z

Adds initial definitions and hooks for ReportingObserver for #29.

Fixes a number of definitions so that bikeshed can separate "Report" the IDL interface from "report" the abstract definition.

This resolves all remaining Bikeshed warnings

RByers · 2017-09-01T19:57:44Z

@mikewest @patrickkettner @paulmeyer90 what do you think of this? I hook into the queuing logic here and invoke this observer-specific machinery. All changes elsewhere in the spec should be unobservable I believe - they're just to resolve definition issues and make bikeshed happy.

I'm starting to work on some web platform tests - there are a surprising number of observable edge conditions to validate. Next major step is to define deprecation reports, but I wanted to see if the basic infrastructure looked OK to you first.

igrigorik · 2017-09-08T03:00:18Z

index.src.html

+  A {{ReportingObserver}} object can be used to observe some types of
+  <a>reports</a> from JavaScript when they are created.
+
+  Issue: How do we specify which reports are observable?  Network-layer reports


In theory, that should be specified when the report is queued, as a default "off" parameter?

I think network-only reports should be the exception. So how about a default-off "network-only" parameter?

igrigorik · 2017-09-08T03:02:31Z

index.src.html

+  1. If present, remove the <a>context object</a> from the list of
+     <a>registered reporting observers</a> for the <a>environment settings
+     object</a> associated with the <a>context object</a>.
+


Based on experience with PerformanceObserver (see w3c/performance-timeline#74), we should also support takeRecords().

Another thing to consider: what about reports queued during "boot time" of the page, before observer can be registered? E.g. it's best practice to remove analytics and other non-critical work from the critical path, but that means that I may miss a whole lot of reports.

For PerfObserver, we're adding support for buffering events up to a certain point: w3c/performance-timeline#81

Thanks!
Discussed takeRecords as part of https://github.com/WICG/reporting/issues/58.
Filed https://github.com/WICG/reporting/issues/59 to discuss boot-time.

igrigorik · 2017-09-08T03:07:00Z

index.src.html

+        [=report/url=], and {{Report/body}} initialized to |report|'s
+        [=report/body=].
+
+        Issue: how to polymorphically initialize body?


Isn't body a JSON payload?

No, it's just an object (i.e. you can do "report.body.message"). For the HTTP path, this object gets serialized to JSON. That said @patrickkettner suggests that the text I have here is fine. We'll have, eg. a DeprecationReport interface which is an instance of ReportBody. It's fine to have text elsewhere that creates a DeprecationReport and then manipulate it here as ReportBody. WDYT?

igrigorik · 2017-09-08T03:09:39Z

index.src.html

+
+     3. Add |r| to the end of |observer|'s <a>report list</a>.
+
+  2. <a>Queue a task</a> to [[#invoke-observers]] with a copy of the


This should probably be queued as a low priority task? For PerfObserver we define own task source and indicate that it should be a low priority queue: https://w3c.github.io/performance-timeline/#queue-a-performanceentry

Thanks, filed https://github.com/WICG/reporting/issues/58 to discuss further.

Add report buffer and |futureOnly| option.

paulmeyer90 · 2018-01-25T16:30:34Z

@igrigorik I've made some changes to ReportingObserver, specifically around:

How observers are registered.
How reports are buffered before being sent to observers, and then queued in the observers before being reported.
The interaction between the |futureOnly| options and the report buffer.
Moving the setting of options from inside the observe() function to directly in the construction of the observer. Note that this prevents a number of edge cases that could arise from changing options after reports have already been observed or reported. Also, I don't think setting |futureOnly| actually makes sense after an observer is already observing, and likewise I don't see the need to change the set of report types to be observed after an observer has already been observing other types.
Nailed down a number of the algorithms to ensure that everything is behaving consistently, especially with respect to the lifetime of reports, and how they are buffered/queued, depending on the options.

Any thoughts?

igrigorik

There is a lot of formatting updates here. Could we split those out into a separate PR? Also, we have https://github.com/WICG/reporting/pull/67 in flight which will cause merge conflicts. That aside...

If I'm reading this correctly, the proposal is to have a global buffer of up to 100 reports, which can be delivered to new observers if they pass in an optional flag in the constructor?

igrigorik · 2018-02-04T00:42:52Z

index.src.html

-                 { "<a>url</a>": "https://backup.com/reports",
-                   "<a>group</a>": "endpoint-1",
-                   "<a>max-age</a>": 10886400 }
+      <a>Report-To</a>: { "<a for="ReportTo">url</a>": "https://example.com/reports",


As a heads up, @dcreager is working on some updates that will break this.

igrigorik · 2018-02-04T00:47:18Z

index.src.html

+  JavaScript, and is represented in JavaScript by the {{ReportingObserver}}
+  object.
+
+  Each <a>environment settings object</a> has a <dfn>registered reporting


For PO we use "Each ECMAScript global environment has": https://w3c.github.io/performance-timeline/#performance-timeline

Okay, I'll use "ECMAScript global environment" instead.

igrigorik · 2018-02-04T00:49:32Z

index.src.html

+  Each <a>environment settings object</a> has a <dfn>report buffer</dfn>, which
+  is an ordered list of <a>reports</a> that have been generated in
+  that <a>environment settings object</a>. This list is initially empty, and the
+  reports are stored in the same order in which they are generated.


For PO we queue the pending entries / reports into each observer, instead of keeping a global buffer:
https://w3c.github.io/performance-timeline/#queue-a-performanceentry

This buffer is just used for the purpose of passing past reports into new observers. Pending reports are still queued on observers (in their "report list").

Is this to handle a race condition, where an observer might be registered after some of the events that it needs to monitor? If so I suggest some explanatory text somewhere; the name buffered implied to me something about queueing up reports between when Reporting receives from the downstream spec and before they're handed over to the observer.

igrigorik · 2018-02-04T00:50:43Z

index.src.html

+  <h3 id=interface-reporting-observer>Interface {{ReportingObserver}}</h3>
+
+  <pre class="idl">
+[NoInterfaceObject]


The body is a JS object, right? Should this really be NoInterfaceObject?

Yes, I'll remove those tags.

igrigorik · 2018-02-04T00:52:04Z

index.src.html

+[Constructor(ReportingObserverCallback callback, optional ReportingObserverOptions options)]
+interface ReportingObserver {
+  void observe();
+  void disconnect();


We should also define takeRecords for consistency — you'll need it. See w3c/performance-timeline@aecfdf4.

Okay, added.

igrigorik · 2018-02-04T00:54:01Z

index.src.html

+
+dictionary ReportingObserverOptions {
+  sequence&lt;DOMString> types;
+  boolean futureOnly = false;


For sake of consistency, s/futureOnly/buffered?

https://w3c.github.io/performance-timeline/#performanceobserverinit-dictionary

I'll change it to |buffered|, but what do you think about the default value. Should reports really be buffered by default in this case? I'm leaning toward changing this to default not buffered both for consistency and because I feel that would likely be the assumption made when omitting the option entirely.

igrigorik · 2018-02-04T00:55:16Z

index.src.html

+};
+  </pre>
+
+  Issue: How do we specify which reports are observable?  Network-layer reports


It seems that each report type should be explicitly stating if it's meant to be JS-observable. We can make this a flag on the ~queue the report step, with default value set to "false".

Okay, I've added a step to check for this. Each specific report type will need to explicitly state that it is observable from JavaScript in order to be observed by a ReportingObserver.

The parameter would need to be per-report, not per-type. Only some Network Error Logging reports should be visible to observers, for instance. In particular, a NEL report should only be visible to the same origin that the report is about. So for instance, if a page at foo.com contains a link to bar.com, bar.com can request NEL reports, but we won't send those reports about bar.com to any foo.com Reporting collectors. And so any Reporting observers registered for foo.com shouldn't be able to see those bar.com NEL reports, either.

I think we can still get that with the parameter that the downstream spec has to set, indicating whether individual reports are visible to observers or not. NEL should have all the info needed to fill that in — if the environment settings object's origin is different than the origin of the request that the report describes, set it to no; otherwise set it to yes. I'd appreciate someone checking my work on that, though. 😉

So in addition to the parameter being present, I'd suggest some clarifying text. ("Downstream specs should make sure to set this parameter so that observers are only able to see reports that would be sent to collectors for that origin" or something like that)

igrigorik · 2018-02-04T00:58:12Z

index.src.html

+
+  4. Set <a>context object</a>'s {{futureOnly}} <a>option</a> to true.
+
+  5. For each |report| in the <a>report buffer</a> associated with |settings|,


This implies that we must queue all reports indefinitely for the lifetime of the page. Is that the behavior we want, and/or reason for concern? It's not crazy to imagine hundreds of reports sitting around indefinitely..

This is so that observers can be made to observe reports that were generated before they could be registered, and this is the behavior we want. We don't expect that the number of reports will be that large, but there is also the buffer limit of 100 in place, so there can't be hundreds sitting around.

There's a section further down about garbage collection, which gives the user agent the ability to throw away reports before they're delivered if they've been sitting around too long. I suggest updating that text to say that if any reports are ever GCed from the reporting cache, they must also be removed from any observer's report list that they appear in.

igrigorik · 2018-02-04T00:59:33Z

index.src.html

+
+  4. Return |observer|.
+
+  The <dfn method for=ReportingObserver><code>observe()</code></dfn>


This definition doesn't cover the case where I call observe() on an existing observer with different options.

See https://w3c.github.io/performance-timeline/#the-performanceobserver-interface for an example.

That issue is not present here because this spec no longer allows options to be passed into observe() for ReportingObserver. The options are passed in on construction of the observer, and so cannot be changed with multiple calls to observe(), for example.

igrigorik · 2018-02-04T01:00:46Z

index.src.html

+
+  2. Add |report| to the <a>report buffer</a> associated with |settings|.
+
+  3. If the <a>report buffer</a> now contains more than 100 reports, remove the


This implies there is a global 100 reports limit?

What is the "beginning"? We should spell it out.

Yes, there is a 100 report buffer limit.

The "beginning" of the list is used here: https://infra.spec.whatwg.org/#lists, so I think it is unambiguous. If it does need specifying though, then it should probably be specified in infra rather than in this spec.

igrigorik · 2018-02-04T01:04:01Z

Paging @dcreager for review :)

paulmeyer90 · 2018-03-08T18:22:31Z

The formatting changes were actually necessary in order to allow bikeshed to properly link everything with the new changes, and so cannot be separated out from this CL.

dcreager

Sorry it took so long to get to this! Broadly speaking I like this; just have several suggestions that all boil down to "needs more intro / explanatory text"

dcreager · 2018-04-02T14:10:00Z

index.src.html

+
+  4. Set <a>context object</a>'s {{futureOnly}} <a>option</a> to true.
+
+  5. For each |report| in the <a>report buffer</a> associated with |settings|,


There's a section further down about garbage collection, which gives the user agent the ability to throw away reports before they're delivered if they've been sitting around too long. I suggest updating that text to say that if any reports are ever GCed from the reporting cache, they must also be removed from any observer's report list that they appear in.

dcreager · 2018-04-02T14:12:18Z

index.src.html

+  Each <a>environment settings object</a> has a <dfn>report buffer</dfn>, which
+  is an ordered list of <a>reports</a> that have been generated in
+  that <a>environment settings object</a>. This list is initially empty, and the
+  reports are stored in the same order in which they are generated.


Is this to handle a race condition, where an observer might be registered after some of the events that it needs to monitor? If so I suggest some explanatory text somewhere; the name buffered implied to me something about queueing up reports between when Reporting receives from the downstream spec and before they're handed over to the observer.

dcreager · 2018-04-02T14:23:23Z

index.src.html

+};
+  </pre>
+
+  Issue: How do we specify which reports are observable?  Network-layer reports


The parameter would need to be per-report, not per-type. Only some Network Error Logging reports should be visible to observers, for instance. In particular, a NEL report should only be visible to the same origin that the report is about. So for instance, if a page at foo.com contains a link to bar.com, bar.com can request NEL reports, but we won't send those reports about bar.com to any foo.com Reporting collectors. And so any Reporting observers registered for foo.com shouldn't be able to see those bar.com NEL reports, either.

I think we can still get that with the parameter that the downstream spec has to set, indicating whether individual reports are visible to observers or not. NEL should have all the info needed to fill that in — if the environment settings object's origin is different than the origin of the request that the report describes, set it to no; otherwise set it to yes. I'd appreciate someone checking my work on that, though. 😉

So in addition to the parameter being present, I'd suggest some clarifying text. ("Downstream specs should make sure to set this parameter so that observers are only able to see reports that would be sent to collectors for that origin" or something like that)

paulmeyer90 · 2018-04-04T19:16:13Z

@dcreager: Not sure why I can't reply to your comments directly. Anyways:

I have added a note to the garbage collection section as you described.
I have added a note to describe the purpose of the |report buffer| and |buffered| option.
Currently, the spec is set up to not allow any reports to be observable cross-origin; they are only sent to observers in the same environment settings object. I think that this property, in addition to specifying which report types are observable, should be sufficient. Are there any other problem cases you can think of that would require observability to be specified per-report rather than per-type? I have also added a note to clarify this in the spec.

dcreager

New text looks great.

Is it worth calling out that the observer part is only relevant for a user agent that has a JavaScript engine? I'm implementing a Reporting client over in nel-reporter-java, for instance, that will be used in native apps that are just using HTTP as a transport mechanism, and don't have any of the other Web Platform bits available. @igrigorik, is there already boilerplate text about that that we can steal from other specs? Or is it implicit and not worth mentioning?

dcreager · 2018-04-06T19:44:39Z

index.src.html

+
+dictionary ReportingObserverOptions {
+  sequence&lt;DOMString> types;
+  boolean buffered = true;


Can we change this to processAlreadyBufferedReports (or something in that vein)? Unless there's a general preference in these specs for shorter names, I like the more descriptive style, especially for boolean options.

I'd prefer current name because it allows us to keep consistency between *Observer interfaces — e.g. PerformanceObserver, ReportingObserver, etc.

igrigorik · 2018-04-06T20:54:30Z

@igrigorik, is there already boilerplate text about that that we can steal from other specs? Or is it implicit and not worth mentioning?

I'm not aware of boilerplate, but you may want to add a note (non-normative, probably) that outlines how non UA clients should behave.

dcreager

LGTM pending some quick text about non-UA clients

dcreager · 2018-04-09T13:34:49Z

index.src.html

+
+dictionary ReportingObserverOptions {
+  sequence&lt;DOMString> types;
+  boolean buffered = true;


RByers added 5 commits August 17, 2017 17:27

Initial skeleton of observer support

f6eff7b

Reporting observer: add key algorithms

1c59754

Don't use IDL references for "report" the concept

c0e690e

Fixes a number of definitions so that bikeshed can separate "Report" the IDL interface from "report" the abstract definition.

Explicitly connect abstract reports to IDL Reports

45391af

This resolves all remaining Bikeshed warnings

Remove obsolete issue

1924bf2

Minor editorial tweaks

4972871

igrigorik reviewed Sep 8, 2017

View reviewed changes

RByers mentioned this pull request Sep 13, 2017

ReportingObserver w3ctag/design-reviews#195

Closed

5 tasks

This was referenced Nov 10, 2017

Specify some report types to be "unobservable"? #57

Closed

Use low priority task queue and add takeRecords to ReportingObserver? #58

Closed

Buffer reports for handling "boot-time" with ReportingObserver? #59

Closed

paulmeyer90 mentioned this pull request Dec 5, 2017

Add ReportingObserver #65

Closed

paulmeyer90 added 5 commits December 12, 2017 13:44

Add report buffer and |futureOnly| option.

702d56f

addressed all new comments

3451bed

Addressed comments.

6a1a57d

Addressed new comments

7d5bff5

Merge pull request #1 from paulmeyer90/observer

058bf2a

Add report buffer and |futureOnly| option.

igrigorik reviewed Feb 4, 2018

View reviewed changes

Addressed comments.

d74bd6b

Merge remote-tracking branch 'upstream/master' into observer

b35fc74

paulmeyer90 mentioned this pull request Mar 9, 2018

Add tests for ReportingObserver. web-platform-tests/wpt#9946

Open

Fixed link errors in bikeshed.

aa3aa40

dcreager reviewed Apr 2, 2018

View reviewed changes

Addressed comments by @dcreager

3a09204

dcreager reviewed Apr 6, 2018

View reviewed changes

dcreager approved these changes Apr 9, 2018

View reviewed changes

index.src.html

dictionary ReportingObserverOptions {

sequence<DOMString> types;

boolean buffered = true;

Copy link

Member

dcreager Apr 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Add notes about non UA clients

79942d6

paulmeyer90 force-pushed the observer branch from 2d4109f to 79942d6 Compare April 9, 2018 17:55

paulmeyer90 merged commit a77a5e5 into w3c:master Apr 9, 2018

patrickkettner deleted the observer branch May 3, 2018 03:38


		3. Add \|r\| to the end of \|observer\|'s <a>report list</a>.

		2. <a>Queue a task</a> to [[#invoke-observers]] with a copy of the


		4. Set <a>context object</a>'s {{futureOnly}} <a>option</a> to true.

		5. For each \|report\| in the <a>report buffer</a> associated with \|settings\|,


		4. Return \|observer\|.

		The <dfn method for=ReportingObserver><code>observe()</code></dfn>


		2. Add \|report\| to the <a>report buffer</a> associated with \|settings\|.

		3. If the <a>report buffer</a> now contains more than 100 reports, remove the

Add ReportingObserver #47

Add ReportingObserver #47

Conversation

RByers commented Sep 1, 2017 • edited by pr-preview bot Loading

RByers commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RByers Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmeyer90 commented Jan 25, 2018

igrigorik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmeyer90 Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrigorik commented Feb 4, 2018

paulmeyer90 commented Mar 8, 2018

dcreager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmeyer90 commented Apr 4, 2018

dcreager left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrigorik commented Apr 6, 2018

dcreager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RByers commented Sep 1, 2017 •

edited by pr-preview bot

Loading

RByers Nov 11, 2017 •

edited

Loading

paulmeyer90 Feb 27, 2018 •

edited

Loading

dcreager left a comment •

edited

Loading