w3c / server-timing

Server Timing
http://w3c.github.io/server-timing/
Other
75 stars 20 forks source link

Incorporating feedback from TAG review #32

Closed yoavweiss closed 7 years ago

yoavweiss commented 7 years ago

Following the TAG review and a meeting which discussed that feedback (and after discussing this more with @cvazac), a few points were raised where the current specification doesn't cover significant use-cases and doesn't lend itself to be extensible in a reasonable way:

Going back to the use cases, I think we need to expand on them and make sure the potential use-cases are either covered, or can be easily and naturally covered by future extensions of the API.

Such potential use-cases that we thought of include:

So, in order to make the syntax more extensible, it might be worthwhile to go back to something slightly like @reschke's proposal and enable multiple parameters.

Maybe something like: Server-Timing: name="response-head"; duration=10; start=5; description="Response head sending"; server="django" Server-Timing: name="response-body"; duration=50; start=130; description="Response body sending"; server="django" Server-Timing: name="db-query"; duration=115; start=15; description="DB query"; server="django" Server-Timing: name="response"; start=7; duration=190; description="Overall response time"; server="nginx"

The above will enable us to tell the visualizer the "story" of the request: the Web server's overall sending time was 190ms where the first byte was received and sent to the user after 7ms. On the application server however, we saw early flush of the head, followed by a DB query, followed by sending of the body.

Such a parameterized approach will also enable us to later on add parameters such as bytes, bool, string, blob, etc in a backwards-compatible way.

/cc @igrigorik @triblondon @slightlyoff

cvazac commented 7 years ago

For v1: For future extensibility concerns, I propose that we shift to named parameters.

Params: name - required string duration - optional length of time (in ms) description - optional string

Examples:

// indicates a cache hit
Server-Timing: name=cache-hit

// a db query took 100ms
Server-Timing: name=db-query; duration=100; description="i love sql"

// denotes requested image is 1024 bytes
Server-Timing: name="disk-size"; description=1024

Devtools and performance analytics scripts need only visualize those entries that contain a duration.

Note: Because name is the only required parameter, we could omit name= in the syntax. A "foo" entry could be just:

Server-Timing: foo; duration=200

For v2: If we are going to make server-timing into more of tracing framework, we need to solve the "epoch problem". I suggest that the epoch on a machine be the point in time that it received the request (earliest timestamp available). Start and end times will be given as relative to the machine epoch, allowing us to be able to draw a per-machine waterfall. A server timing entry will describe a processing step on one machine, not across machines.

Params: start - optional relative time (in ms) end - optional relative time (in ms) source - optional unique server/service identifier (examples: expressjs, expressjs-192.168.1.5, 192.168.1.5)

Examples:

// denotes length of time:
Server-Timing: name=foo; start=100; end=200
Server-Timing: name=foo; start=100; duration=300
Server-Timing: name=foo; end=200; duration=300

// denotes point in time:
Server-Timing: name=foo; start=100
Server-Timing: name=foo; start=100; duration=0
Server-Timing: name=foo; end=200;
Server-Timing: name=foo; end=200; duration=0

// this is not legal
Server-Timing: name=foo; start=100; end=200; duration=300

Devtools and performance analytics scripts need only visualize those entries that contain a duration | start | end.

Note: end is provided for flexibility, it is definitely not needed.

For v3: Hand-wavy ideas that we can tackle:

Params: unit - enum, default is ms data - arbitrary blob source-index (to indicate order of server/service)

igrigorik commented 7 years ago

Server-Timing: foo; duration=200

Based on offline discussions @ BlinkOn, v1 sgtm — name is redundant. Also, I'd propose we split v1+ discussions into a separate thread and tackle those separately and after resolving the v1 update we're discussing here.

triblondon commented 7 years ago

TAG took another look at this in Nice:

In general, we're really happy that you were able to accommodate our earlier feedback and we think this will be a great benefit to developers. Thanks!

cvazac commented 7 years ago

We are wondering about the parsing rules, sometimes strings are quoted, sometimes not.

I think we should allow all named params (including duration - for consistency) to be optionally wrapped in DQUOTES. Best I can tell, that's analogous to the LINK header.

'description' might be better called 'value', since it is no longer semantically a description of the duration value

description describes the metric itself, not the duration value. I'm inclined to leave as description, unless maybe I'm missing your point.

reschke commented 7 years ago

Please ignore the specific syntax rules in RFC 5988; it's going to be obsoleted any day now: https://tools.ietf.org/html/draft-nottingham-rfc5988bis-08

igrigorik commented 7 years ago

With https://github.com/w3c/server-timing/pull/37 landed, can we resolve this?

cvazac commented 7 years ago

Future considerations enumerated here.