Add Server-Timing header to unread page - #806
Conversation
fguillot
left a comment
fguillot
left a comment
There was a problem hiding this comment.
This feature should be optional (disabled by default). It should have a config option to enable the timing headers. To avoid adding if/else everywhere, it could be done at the middleware level only.
I would like to suggest different names:
- Not sure what
prepmeans, perhaps something likepre_processingmight be more explicit. sql_count_unread_entriessql_fetch_unread_entriestemplate_rendering
|
To deactivate headers by default, I could make a PR to the upstream library to add an option disabling healders: uiRouter.Use(func(h http.Handler) http.Handler {
return servertiming.Middleware(h, servertiming.MiddlewareOpts{DisableHeaders: true})
})This would ensure that headers are not written in the final requests, even if the measurements would be gathered. I’ve applied the suggested names. |
|
Sure, if they accepts the PR it's ok. |
This option prevents metrics from being written in the corresponding header of the response. This is useful to collect the metrics all the time but have a setting to actually write it in the answer. For instance, this is desirable for miniflux/v2#806
1ce3a09 to
35f108c
Compare
|
@fguillot I think this PR is ready for you to take another look, when you have time. |
d03bcf3 to
373f952
Compare
| p.opts.logDateTime = parseBool(value, defaultLogDateTime) | ||
| case "DEBUG": | ||
| p.opts.debug = parseBool(value, defaultDebug) | ||
| case "SERVER_TIMING_HEADER": |
There was a problem hiding this comment.
I can see you have updated the variable here, but all occurrences should be updated as well.
There was a problem hiding this comment.
Sorry, it should be fixed now.
Unless you meant to change the timing field as well?
373f952 to
0df0a06
Compare
| httpService bool | ||
| schedulerService bool | ||
| debug bool | ||
| timing bool |
There was a problem hiding this comment.
It would be nice to rename this one as well, that would make it explicit.
dbe2ff8 to
1b99598
Compare
To be used as a basis to discuss miniflux#722. Current metrics may not be the most interesting ones. This is not deactivable at the moment, the header is: ``` server-timing: prep;dur=30.548655,sql_unread;dur=8.762088,sql_entries;dur=16.670605,render;dur=566.738683 ``` The construction advised by the library ``` defer timing.NewMetric(name).Start().Stop() ``` does not work, the metric has a duration of 0s. I suspect this is due to the request being finalized too early for the header to be added. Some more granular metrics as added in this patch may be more relevant anyway.
1b99598 to
f66aed5
Compare
|
I think it is ready for you to take another look @fguillot. |
2 participants
To be used as a basis to discuss #722. Current metrics may not be the
most interesting ones.
This is not deactivable at the moment, the header is:
Do we want this feature to be configurable?
The construction advised by the library
does not work, the metric has a duration of 0s. I suspect this is due to
the request being finalized too early for the header to be added. Some
more granular metrics as added in this patch may be more relevant anyway.
Example result in browser:
