1403051 - Add support for w3c Server-timing headers

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Zac Spitzer]

Server-timing headers is a new w3c standard for including statistics in the headers of a http response

https://w3c.github.io/server-timing/
https://ma.ttias.be/server-timings-chrome-devtools/
https://gist.github.com/paulirish/a76ac17fc211b019e538c09d8d827691

It just requires a header to be parsed and values to be displayed in the timings panel

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report!

Honza

Comment 2

[garret.rus]

Attachment: Bug1403051.patch

I've tried to add this feature (first bug).
Confusing things:
* Netmonitor could not start after a global component rename (app -> App)
* Netmonitor could not properly render after a recent connector refactoring (sending the props as a fifth argument is not supported in the devtools-launchpad@0.0.96)
* Firebug theme have missed colors in timing, or I just can't build it properly

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8920871 [details] [diff] [review]
Bug1403051.patch

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

The patch looks good. I think it goes in the right direction.

* Netmonitor could not start after a global component rename (app -> App)
* Netmonitor could not properly render after a recent connector refactoring (sending the props as a fifth argument is not supported in the devtools-launchpad@0.0.96)
This should be fixed for OSX, see bug 1410782

* Firebug theme have missed colors in timing, or I just can't build it properly
Firebug theme will be removed soon see bug 1378108


Couple of questions:
* Do you have an online test page that is sending some server-timings?
* Do you see any potential performance issues (e.g. parsing headers?)

Thanks for working on this!

Honza

Comment 4

[garret.rus]

> Do you have an online test page that is sending some server-timings?
My actual project uses server-timing only in dev environment, so I don't know any online page with server-timing.
I've tested patch on that dev version and simple node.js test server.

> Do you see any potential performance issues (e.g. parsing headers?)
Maybe I'm missing something, but I've seen only one bad thing here - sort with toLowerCase call in each comparison.
Did you find any other thing that should fixed?

Also seems this is a duplicate bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1337684

> Thanks for working on this!
Thanks for review :)

Comment 5

[Jason Duell]

Note that since Server-Timing is an HTTP *trailer* (at end of the payload instead of the beginning), it will need some special support from necko.  I've filed bug 1413999 for that.

It does look like bug 1337684 is a dupe of this one?  If not, perhaps it's the one that needs to depend on bug 1413999?

Comment 6

[Kershaw Chang [:kershaw]]

Hi Honza,

I am currently working on bug 1413999, which is adding support for server timing in necko.
In bug 1413999, I tend to implement the header parser in necko code, but I also see that the code for parsing header is also appeared in the patch in this bug. I am just wondering is this common to parse http headers in js? Do you need only a header string or the parsed data?

Thanks.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Kershaw Chang [:kershaw] from comment #6)
> Hi Honza,
> 
> I am currently working on bug 1413999, which is adding support for server
> timing in necko.
> In bug 1413999, I tend to implement the header parser in necko code, but I
> also see that the code for parsing header is also appeared in the patch in
> this bug. I am just wondering is this common to parse http headers in js?
I think so, DevTools are parsing header e.g. to get content type,
encoding, etc.

> Do you need only a header string or the parsed data?

It's probably useful to have access to both. For example, one of the side
panels in the Network monitor panel shows parsed (and nicely formatted)
Headers as well as raw data (to see what *exactly* came over the wire
from the server).

Honza

Comment 8

[Kershaw Chang [:kershaw]]

> > Do you need only a header string or the parsed data?
> 
> It's probably useful to have access to both. For example, one of the side
> panels in the Network monitor panel shows parsed (and nicely formatted)
> Headers as well as raw data (to see what *exactly* came over the wire
> from the server).
> 

Could you let me know how do you want to get the data and what is the format you expected?
In bug 1413999, I plan to add nsIServerTiming interface and a nsIArray of nsIServerTiming in nsITimedChannel. The idl would be like:

interface nsIServerTiming : nsISupports {
  [must_use] readonly attribute ACString name;
  [must_use] readonly attribute double duration;
  [must_use] readonly attribute ACString description;
};

interface nsITimedChannel : nsISupports {
  readonly attribute nsIArray serverTiming;
};

Is this enough to you?

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Kershaw Chang [:kershaw] from comment #8)
> Could you let me know how do you want to get the data and what is the format
> you expected?
> In bug 1413999, I plan to add nsIServerTiming interface and a nsIArray of
> nsIServerTiming in nsITimedChannel. The idl would be like:
> 
> interface nsIServerTiming : nsISupports {
>   [must_use] readonly attribute ACString name;
>   [must_use] readonly attribute double duration;
>   [must_use] readonly attribute ACString description;
> };
> 
> interface nsITimedChannel : nsISupports {
>   readonly attribute nsIArray serverTiming;
> };
> 
> Is this enough to you?
This looks good and nicely corresponds to the spec [1], so yes should be enough.

Since the server-timings are transferred using HTTP trailer not a header, is there any plan to have API that can be used (from JS) to access raw representation of all HTTP trailers? This could be also visualized for the user in DevTools Toolbox. I understand that server-timings is the only one supported, correct?

Honza


[1] https://w3c.github.io/server-timing/#dfn-server-timing-header-field

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> (In reply to Kershaw Chang [:kershaw] from comment #8)
> > Could you let me know how do you want to get the data and what is the format
> > you expected?
> > In bug 1413999, I plan to add nsIServerTiming interface and a nsIArray of
> > nsIServerTiming in nsITimedChannel. The idl would be like:
> > 
> > interface nsIServerTiming : nsISupports {
> >   [must_use] readonly attribute ACString name;
> >   [must_use] readonly attribute double duration;
> >   [must_use] readonly attribute ACString description;
> > };
> > 
> > interface nsITimedChannel : nsISupports {
> >   readonly attribute nsIArray serverTiming;
> > };
> > 
> > Is this enough to you?
> This looks good and nicely corresponds to the spec [1], so yes should be
> enough.
> 
> Since the server-timings are transferred using HTTP trailer not a header, is
> there any plan to have API that can be used (from JS) to access raw
> representation of all HTTP trailers? 

I think I can file another bug for accessing HTTP trailers.

> This could be also visualized for the
> user in DevTools Toolbox. I understand that server-timings is the only one
> supported, correct?
> 

Yes, we will only put server-timings in trailers for now.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Kershaw Chang [:kershaw] from comment #10)
> I think I can file another bug for accessing HTTP trailers.
Excellent!

Please cc me (or cross-link here) the bug, so I can track the progress.

Thanks,
Honza

Comment 12

[Jason Duell]

FYI: the non-devtools work for server-timing is getting close, so we should look into moving this bug forward again soon.

Comment 13

[u408661]

Just to followup on comment 11, we have HttpBaseChannel::GetResponseTrailers for the purpose of getting the trailer array from the channel, so necko side should be good to go here.

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@garret: It's now possible to get HTTP trailers (see comment # 13),
and so we can move forward with this bug.

Are you still interested in it?
Should I assign the bug to you to finish it?

Honza

Comment 16

[Sebastian Zartner [:sebo]]

Garret wasn't active on Bugzilla for a long time, so I clear the ni.

And for what it's worth, somebody on Discourse was wondering why this feature isn't available yet.

Sebastian

Comment 17

[:Harald Kirschner :digitarald]

Would this be a good first bug?

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Perhaps good first bug for upcoming internship (Outreachy #18)

Honza

Comment 19

[Silent Imp]

Any news?
It's still frustrating and actually, it's hard to implement it in the production due to lack of FF support.
«Not enough value to add this to the project» :(

Comment 21

[Sorin Davidoi]

Attachment: Bug 1403051 - Server Timings. r=honza

Uses mock data to show the server timings in the Timings panel.

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Sorin: Thanks for working on this! Here is the promised info:

So, the missing piece is sending the data to the place where it's rendered. Note that DevTools are based on client-server architecture (or client/backend).

• The part that is getting the serverTiming data from HTTP channel is the backend (or DevTools server).
• The part that is rendering the data is the clients (or front-end)

The communication between these two parts is done through RDP (remote debugging protocol)
We just send packets of JSON back and forward.

Details data about individual HTTP requests are fetched lazily on demand so, we keep the RDP traffic low.

The fetching usually happens when a side panel is opened e.g. in case of the Timings panel, it's here:
https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/devtools/client/netmonitor/src/components/TimingsPanel.js#36

So, there is already a packet type that is used to sending timing data.
I think that we can use it and extend it to convey the additional server timing data.

We were doing something similar when sending additional isRacing field through security packet type

So, you need to study this patch:
https://github.com/mozilla/gecko-dev/commit/67808459acd383c95ea1c69c6cf69a55d719d0d0

Some more comments:

1. Here is the method call where we add timings data
   https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/devtools/server/actors/network-monitor/network-observer.js#418

and here is the impl:
https://searchfox.org/mozilla-central/source/devtools/server/actors/network-monitor/network-event.js#528

You need to extend it and add additional server timings data

2. You also need to extend the packet type since it'll contain some new fields

https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/devtools/shared/specs/network-event.js#52-66

See all the examples in the files to learn how it works.

3. With little bit of luck you should get the data in the panel as part of the existing packet:

https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/devtools/client/netmonitor/src/components/TimingsPanel.js#45

It should go through this place so, you might want to log something in case of issues:
https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/devtools/client/netmonitor/src/connector/firefox-data-provider.js#759

Honza

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Btw. what test page should I use for testing?
Honza

Comment 24

[Sorin Davidoi]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #23)

Btw. what test page should I use for testing?
Honza

https://www.bbc.co.uk/iplayer

Comment 25

[Sorin Davidoi]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #22)

@Sorin: Thanks for working on this! Here is the promised info:

Thank you, they were just what I needed. Pushed another patch to use real data.

Comment 26

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

Harald, the patch here is almost ready. Do you have any comments on the UI?
A suggestion how the timings could be rendered in the Timings panel is attached.
Honza

Comment 27

[:Harald Kirschner :digitarald]

Looks great, maybe we can use more subtle headers?

Inspector has these rules in the Grid panel:

.grid-container > span {
	font-weight: 600;
	margin-bottom: 5px;
	pointer-events: none;
}

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: server-timings.diff

@ sorin: I am attaching a patch that solves the remaining backend issues:

• support for timings in headers as well as trailers
• using HTTPS for the test
• small CSS update
• adopting the test

Please look at my changes and merge with your patch on phab.
This is very close to landing.

Thanks for the help!
Honza

Comment 29

[Sorin Davidoi]

That looks great! Will probably only get the chance to merge it with my patch next Thursday/Friday though.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c049e82eb05
Server Timings. r=Honza

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d93010798ea5
Fix ES lint failure

Comment 32

[Arthur Iakab [arthur_iakab]]

Backed out 2 changesets (bug 1403051) for causing devtools failures on browser_parsable_css.js CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/e729376dff049ba4f17cf814203117f8a58e237c

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=268561440&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=0c049e82eb0532e37ae3426a5ad998b32b81b348

Failures https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&searchStr=OS%2CX%2C10.14%2CShippable%2Copt&fromchange=ea85e72e5ebe6a4e1cf1fef1aa347ae0f9ee82ee&selectedJob=268563330

Failure logs https://treeherder.mozilla.org/logviewer.html#?job_id=268560716&repo=autoland for dt 4
https://queue.taskcluster.net/v1/task/XR4Zk7nKT8eLKOmaHRqRsQ/runs/0/artifacts/public/logs/live_backing.log for dt3

Honza can you please take a look?

Comment 33

[Sorin Davidoi]

Fixed the tests and the formatting.

Comment 34

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Sorin Davidoi from comment #33)

Fixed the tests and the formatting.

Did you upload it on Phab?

The current version in Phab is still failing since the following CSS vars are not used:

.theme-light .network-monitor .requests-list-timings-box {
  --timing-server-color1: rgba(104, 195, 179, 0.8); /* teal */
  --timing-server-color2: rgba(123, 102, 167, 0.8); /* purple */
  --timing-server-color3: rgba(233, 236, 130, 0.8); /* yellow */
  --timing-server-color-total: rgba(186, 90, 140, 0.8); /* pink */
}

.theme-dark .network-monitor .requests-list-timings-box {
  --timing-server-color1: rgba(74, 228, 201, 0.8); /* teal */
  --timing-server-color2: rgba(156, 119, 233, 0.8); /* purple */
  --timing-server-color3: rgba(234, 239, 73, 0.8); /* yellow */
  --timing-server-color-total: rgba(186, 74, 133, 0.8); /* pink */
}

Comment 35

[Sorin Davidoi]

Did you upload it on Phab?

Yes.

The current version in Phab is still failing since the following CSS vars are not used

They are used, but maybe not in the right way?

backgroundColor: `var(--timing-server-color${color})`

Comment 36

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, I see that. Commented on Phab
Honza

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72d41f43e9b9
Server Timings. r=Honza

Comment 38

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/72d41f43e9b9

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

This seems like a good candidate for a release note, no?

Comment 40

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yes, agree. It's already recorded in the MDN content roadmap: https://trello.com/c/dE8tJ7my/87-network-monitor-enhancements-fx-71

Honza

Comment 41

[:Harald Kirschner :digitarald]

Release Note Request (optional, but appreciated)
[Why is this notable]: Server-timings are used by companies to track performance and it has been a long-standing feature gap.
[Affects Firefox for Android]: No
[Suggested wording]: Using the new server-timing information in DevTools' Network panel, developers can analyze the server-related timings in context of other request and response details.
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor/request_details#Timings

Comment 42

[Steve Roussey (:sroussey)]

Yay! Glad to see this. Kinda feel bad I didn't do it myself though.

One question... does it only support trailing headers? Or is that a just under the hood detail?

Comment 43

[:Harald Kirschner :digitarald]

I am trying to validate with a test case, but it should be a platform detail that isn't exposed to the frontend and should just work.

Comment 44

[Pascal Chevrel:pascalc]

Harald, are you requesting an addition to our general release notes? The relnote-firefox flag is for our main release notes (such as https://www.mozilla.org/en-US/firefox/69.0/releasenotes/). If you only need added to our MDN notes for a release (https://developer.mozilla.org/fr/docs/Mozilla/Firefox/Versions/69) then the dev-doc-needed keyword is enough.

Comment 45

[:Harald Kirschner :digitarald]

Thanks Pascal, I would like to give this a shot for the general release notes as it is interesting for an audience beyond frontend developers, like devops, QA and backend; and it is a long-standing tooling gap where Firefox is the last to ship the UI for inspecting the values. Given that, it should fit well into the Developer section.

(Aside, dev-doc-needed does not necessarely ensure that work makes it into release notes atm, with missing triage.)

Comment 46

[Pascal Chevrel:pascalc]

I added it in Nightly notes so as that we don't forget it for beta notes https://www.mozilla.org/en-US/firefox/71.0a1/releasenotes/

Comment 47

[Pascal Chevrel:pascalc]

This will be in the final release notes.

Comment 48

[Chris Mills [:cmills]]

Hi there,

I'm trying to figure out what MDN documentation needs to be written for this, and I'm not sure. We have this section alread:

https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor/request_details#Timings

Does anything need to be aded to it? If so, what? I see the timings tab has some new parts to it; what bits relate to Server-Timing?

Thanks!

Comment 49

[:Harald Kirschner :digitarald]

Luckily there is some prior art, like: https://ma.ttias.be/server-timings-chrome-devtools/ , https://www.smashingmagazine.com/2018/10/performance-server-timing/#the-server-timing-header and https://umaar.com/dev-tips/136-server-timing/

Comment 50

[Chris Mills [:cmills]]

Thanks :digitarald! This is now documented; see https://github.com/mdn/sprints/issues/2277#issuecomment-561281970 for the full details.

Let me know if you think this needs anything else. Thanks!