Skip to content
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

Incorporating feedback from TAG review #32

Closed
yoavweiss opened this issue Sep 13, 2017 · 7 comments
Closed

Incorporating feedback from TAG review #32

yoavweiss opened this issue Sep 13, 2017 · 7 comments

Comments

@yoavweiss
Copy link
Contributor

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:

  • Right now, the only first class measurement is a "duration". While it is possible to stuff all other measurements into a blob as a "description", that won't enable us to easily standardize used patterns in the future.
  • The lack of startTime (which we discussed) is a major impediment if we want to be able to draw server-side waterfalls. I think the previous discussion thought of this as "start time from navigation", where we should think of it as "start time of current reported duration, relative to the moment in time where the server first read the request". Maybe we should find a better name than "startTime" to avoid confusion with NavTiming.
  • The assumption that all measurements are in milliseconds (as they are assumed to be a duration) would again mean that all other metrics would be stuffed into the description and therefore invisible (or worse, displayed as a blob to the end user). It may be worthwhile to have a first class way to define "units" and have current visualizers (e.g. devtools, analytics) ignore units they do not know how to visualize.
  • We need to realize that there is no single server. The application server, web server, memcached and intermediaries are all likely to add server timing entries, and we'd want to make sense of that. In order to do that, it may make sense to add some "server-id" to each measurement to indicate who took it, and what its "start time" refers do in relation to other measurements.

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:

  • Being able to draw server-side watefalls based on server-timing data, while having multiple servers (with different start times) provide metrics.
  • Be able to send non-duration metrics, explicitly express that in some way, and have visualizers ignore metrics they do not recognize. Examples may include bytes, server/data-center names, booleans (cache hit or miss), indication of resource types/handling (e.g. image on-the-fly vs. offline compression), etc
  • Being able to send blobs of data and not have it visualized, as it would be meaningless.

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
Copy link
Contributor

cvazac commented Sep 13, 2017

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
Copy link
Member

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
Copy link

TAG took another look at this in Nice:

  • We are wondering about the parsing rules, sometimes strings are quoted, sometimes not.
  • 'description' might be better called 'value', since it is no longer semantically a description of the duration value
  • Agree with the 'name=' part being optional
  • We're reserving judgement on v2 and later, but v1 looks good

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
Copy link
Contributor

cvazac commented Sep 28, 2017

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
Copy link

reschke commented Sep 28, 2017

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
Copy link
Member

With #37 landed, can we resolve this?

@cvazac
Copy link
Contributor

cvazac commented Nov 3, 2017

Future considerations enumerated here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants