Closed Bug 1403051 Opened 3 years ago Closed 1 year ago

Add support for w3c Server-timing headers

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(relnote-firefox 71+, firefox71 fixed)

RESOLVED FIXED
Firefox 71
Tracking Status
relnote-firefox --- 71+
firefox71 --- fixed

People

(Reporter: zac.spitzer, Assigned: sorin.davidoi)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [webpagetest])

Attachments

(4 files)

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
Thanks for the report!

Honza
Severity: normal → enhancement
Priority: -- → P3
Attached patch Bug1403051.patchSplinter Review
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
Attachment #8920871 - Flags: ui-review+
Attachment #8920871 - Flags: feedback?(odvarko)
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
Attachment #8920871 - Flags: feedback?(odvarko) → feedback+
> 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 :)
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?
Depends on: 1413999
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.
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
> > 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?
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
(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.
(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
Depends on: 1428250
FYI: the non-devtools work for server-timing is getting close, so we should look into moving this bug forward again soon.
Flags: needinfo?(odvarko)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(odvarko)
@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
Flags: needinfo?(garret.rus)
Product: Firefox → DevTools
Whiteboard: [webpagetest]

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

Flags: needinfo?(garret.rus)

Would this be a good first bug?

Flags: needinfo?(odvarko)

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

Honza

Flags: needinfo?(odvarko)

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» :(

Duplicate of this bug: 1576607
Assignee: nobody → sorin.davidoi
Status: NEW → ASSIGNED

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

@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

  1. 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.

  1. 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

Flags: needinfo?(sorin.davidoi)

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

(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

Flags: needinfo?(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.

Attachment #9092144 - Attachment description: Bug 1403051 - Groudwork for Server Timings. r=honza → Bug 1403051 - Server Timings. r=honza
Attached image 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

Flags: needinfo?(hkirschner)

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;
}
Flags: needinfo?(hkirschner)

@ 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

Flags: needinfo?(sorin.davidoi)

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

Fixed the tests and the formatting.

Flags: needinfo?(sorin.davidoi)

(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 */
}
Flags: needinfo?(odvarko) → needinfo?(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})`
Flags: needinfo?(sorin.davidoi)

Ah, I see that. Commented on Phab
Honza

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

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

Flags: needinfo?(odvarko)

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

Honza

Flags: needinfo?(odvarko)

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

relnote-firefox: --- → ?

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?

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.

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.

Flags: needinfo?(hkirschner)

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.)

Flags: needinfo?(hkirschner)

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/

This will be in the final release notes.

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!

Flags: needinfo?(sorin.davidoi)
Flags: needinfo?(odvarko)

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!

You need to log in before you can comment on or make changes to this bug.