New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review request for Server-Timing #188

Closed
cvazac opened this Issue Aug 14, 2017 · 16 comments

Comments

Projects
None yet
10 participants
@cvazac

cvazac commented Aug 14, 2017

Hello TAG!

I'm requesting a TAG review of:

Further details (optional):

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our Github repo for each point of feedback
  • open a single issue in our Github repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [github usernames]
@dbaron

This comment has been minimized.

Member

dbaron commented Aug 15, 2017

It would perhaps be useful to see an explainer document here, to answer questions such as why a new specification is needed here rather than reusing existing mechanisms. (My guess at the answer to that particular question is so that timing information for all the resources that are used by the document, no matter what type of resources they are, can be included easily in the timeline.)

@slightlyoff

This comment has been minimized.

Member

slightlyoff commented Aug 18, 2017

Would also like to see an explainer; we recently put together a dummy outline that could help in drafting yours

Would also be good to see discussion of security and privacy considerations in the draft spec.

In terms of the design, a few questions:

  • The encoding for these values seems very one-off; does it share a common grammar with other systems (e.g., CSP)? Could it possibly use JSON?
  • The exact parsing semantics of the metric-value (and reflection back to JS) seem ambiguous to me. No formalism (or JS conversion) are offered in the text, only the note that they must be representable as a double, but that leaves a lot of room for interpretation. I'd like to see specific numeric conversion steps outlined.
  • The example code in the spec uses a confusing mix of old and new syntax. A fully modern version might read:
// Example 2
// serverTiming entries can live on 'navigation' and 'resource' entries
['navigation', 'resource'].forEach((entryType) => {
  let entries = performance.getEntriesByType(entryType);
  for(let { name: url, serverTiming } of entries) {
      // iterate over the serverTiming array; only the slow ones
      for(let { name, duration, description } of serverTiming.filter(t => { t.duration > 200 }) {
        console.info('Slow server-timing entry =',
            JSON.stringify({url, entryType, name, duration, description}, null, 2))
      }    
  }
@cvazac

This comment has been minimized.

cvazac commented Aug 21, 2017

Explainer

Security discussion

The header format that we've chosen for Server Timing is similar to Accept-Language and Content-Type

Regarding "specific numeric conversion steps", is there any prior art you can link me to? cc @slightlyoff

Modernized example code.

@mnot

This comment has been minimized.

Member

mnot commented Aug 31, 2017

A few notes on the header ABNF:

  • Omit the header field-name and colon from the Server-Timing rule.
  • Reference RFC7230, Section 7 for the # extension.
  • Reference RFC5234 for ABNF.
  • metric allows OWS around the "=", unlike RFC7231 parameter. Is this intentional?
  • metric-value allows an empty value (e.g., "foo="); is this intentional? If so, it should be called out in prose.
  • Is using non-ascii characters in metric-name and/or description a desired use? If so, you need to define an encoding, e.g., RFC5987bis.

See also the checklist for new header fields.

@cvazac

This comment has been minimized.

cvazac commented Sep 1, 2017

@mnot:

  • Omit the header field-name and colon from the Server-Timing rule.
  • Reference RFC7230, Section 7 for the # extension.
  • Reference RFC5234 for ABNF.

👍 (w3c/server-timing#31)

  • metric allows OWS around the "=", unlike RFC7231 parameter. Is this intentional?

parameter in RFC7231 looks to be specific to Accept and Content-Type. I thought it made sense to allow the optional whitespace for readability. But I'm not finding any prior art for OWS around equal signs... I don't have a problem removing it from spec, but I'd think browsers would still want to allow.

I dug into parameter for Content-Type a little further, and every modern browser supports WS before and after the "=" (excepting Firefox which supports WS after, but not before, the =)

  • metric-value allows an empty value (e.g., "foo="); is this intentional? If so, it should be called out in prose.

I see what you mean, this happened because of a recent change. metric-value is the way it is to allow for foo=.25 (no leading "0"). foo= does not hold more meaning than foo, which we need to be legal. Is there a way you know of in ABNF to tighten that up?

  • Is using non-ascii characters in metric-name and/or description a desired use? If so, you need to define an encoding, e.g., RFC5987bis.

I could have this wrong, but I think both are just ascii, which is what we want. for metric-name:
metric-name is a token (link)
token resolves to atchar, which resolves to asciis, DIGITs, and ALPHAs (link)
ALPHAs are just A-Z / a-z (link)

@triblondon

This comment has been minimized.

triblondon commented Sep 5, 2017

A few thoughts from me informed by recent TAG call:

  • I'm slightly concerned that "people" (ie, me) will use Server-Timing as a mechanism to communicate arbitrary page related metadata from server (or middlebox) to client, because it is the only HTTP header, to my knowledge, that is accessible from JavaScript (for a page navigation), without side effects.
  • It is a shame that there is no start time on a timing metric, so it is impossible to describe the concurrency or blocking of tasks in relation to one another, possibly the most valuable use-case for this header. As an alternative we could have some kind of declared relationship between the metrics to indicate that they block.
  • There's a separate discussion of syntax, my preference would be to drop the semicolon delimiter idea. I can't see the need for a millisecond duration AND a description as a common requirement, so you're often going to get key1=;val, key2=;val2 which is weird and looks wrong. My preference would be to interpret the value as a time period if it includes a unit, CSS style.
@plinss

This comment has been minimized.

Member

plinss commented Sep 5, 2017

@cvazac and @yoavweiss are either (or both) of you available to join a TAG call to discuss this further? We meet at 8am PDT (15:00 UTC) on Sept 12 or 19 would be ideal.

@hadleybeeman

This comment has been minimized.

hadleybeeman commented Sep 5, 2017

Discussed on TAG call 5 Sept 2017, particularly questions around the choice not to use JSON. We had a quick look at @reschke's internet draft A JSON Encoding for HTTP Header Field Values.

We also had a quick look at its data tracker page... What is the status/momentum of this? Is it implemented anywhere?

@mnot, any additional context/input?

@yoavweiss

This comment has been minimized.

yoavweiss commented Sep 5, 2017

@cvazac and @yoavweiss are either (or both) of you available to join a TAG call to discuss this further? We meet at 8am PDT (15:00 UTC) on Sept 12 or 19 would be ideal.

I can join if needed on September 12th.

We also had a quick look at its data tracker page... What is the status/momentum of this? Is it implemented anywhere?

Implemented with intent to ship in Blink. Intent to implement in WebKit.

@cvazac

This comment has been minimized.

cvazac commented Sep 5, 2017

  • AFAIK yes, it's the only header accessible to script, outside of cookies.

  • regarding startTime
    related discussion here
    alternates to startTime as a first-class value (that "work" today):

db-start=100, db-duration=50

db=50; 100

  • regarding syntax:
    the following are all legal:
    key
    key=duration
    key;description (no need for = when no duration specified)
    key=value;description
    @triblondon are you suggesting we go from three fields per metric to two - one for the name of the metric, and the other as a string (which should be interpreted as a number if there are known time-based units)?

I will join on 9/12. Can someone please send me the details for the call? cc @hadleybeeman

@plinss

This comment has been minimized.

Member

plinss commented Sep 5, 2017

Great that you both can make it, we'll be using WebRTC via Jitsi: https://meet.jit.si/w3ctag

@igrigorik

This comment has been minimized.

igrigorik commented Sep 7, 2017

Hey all, lots of great discussions here!

Meta ask and observation: if there is high-level design feedback you think is worth iterating on, please file issues against the spec repo and let's iterate there, where it's visible to other WG members. We shouldn't (re)design features in this forum.

@plinss

This comment has been minimized.

Member

plinss commented Sep 11, 2017

@triblondon

This comment has been minimized.

triblondon commented Sep 12, 2017

Strawman proposal:

Server-Timing: db-start=100;type=moment, db-duration=50;type=duration, peak-memory=4525345;type=bytes, data-center=LHR;type=enum, cache-status=HIT;type=enum, user-id=3457853;type=str

Obviously I can't predict all the different types, but the point is that tooling is going to want to visualise these in different ways, for instance:

  • Durations as horizontal bars
  • Moments as vertical lines / dots
  • Bytes scaled to an appropriate human-readable unit (tool might also conceivably compare the value with values for the same metric for previous requests)
  • ENUMs as tag-style labels, perhaps with automatic colour coding
  • Freefrom strings as just basic tabulated data

It seems not hard to build support for this kind of extensibility into the syntax, so as to avoid this:

Server-Timing: data=;<URL-ENCODED-JSON>

One might imagine the above is "fine" on the basis that it's not interfering with the intended use of the header, but if the tools for which this header is designed start to look for and depend on this syntax, then it would be very hard to introduce a type attribute later on.

I'm not a big fan of adding an explicit description attribute key, because I do ultimately think that a string value should not have to live in a meta property just because you have reserved the main value property for a millisecond duration. However, a meta property seems like a reasonable solution (and an alternative to a unit) to switch the processing model that should apply to the main value.

@yoavweiss

This comment has been minimized.

yoavweiss commented Sep 13, 2017

Started w3c/server-timing#32 to discuss feedback from this review and from yesterday's meeting.

@triblondon

This comment has been minimized.

triblondon commented Sep 27, 2017

Posted w3c/server-timing#32 (comment) to respond to the proposed changes. Everything looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment