Additional stats #1017
Additional stats #1017
Conversation
| { | ||
| int32_t delta_sec = (int32_t)until->tv_sec - (int32_t)from->tv_sec; | ||
| int32_t delta_usec = (int32_t)until->tv_usec - (int32_t)from->tv_usec; | ||
| int64_t delta = (int64_t)(delta_sec * 1000*1000L) + delta_usec; |
There was a problem hiding this comment.
You might want to convert either of delta_sec or 1000*1000L to int64_t before multiplication, so that the operation would be done in 64-bit on ILP32.
|
Thank you for the PR. The design looks fine, and I think the proposed feature would be a great addition to H2O. I have left several comments in-line, please let me know how you think. |
This commit adds duration and jemalloc statistics, both are optional: jemalloc is enabled at compile time and the duration statistics are enabled by a 'duration-stats' configuration keyword. The commit also adds a total number of connections counter. Duration statistics build on the existing per request duration stats that are available for logging, each observation is stored in a structure that allows computing quantiles. When the status page is queried, values for the minimal, 25th, 50th, 75th and 99th percentile are output.
- h2o_timeval_substract - h2o_timeval_is_null - Wrap all jemalloc code in a big `if()` statement
We use this to define how the following durations are computed: `connect_time`, `header_time`, `body_time`, `request_total_time`, `process_time`, `response_time`, `duration`. We then use this both in `durations.c` and `logconf.c` to compute said durations.
86f01d0 to
1dcb989
Compare
| @@ -44,6 +45,21 @@ int h2o_time_parse_rfc1123(const char *s, size_t len, struct tm *tm); | |||
| */ | |||
| void h2o_time2str_log(char *buf, time_t time); | |||
|
|
|||
| static inline int64_t h2o_timeval_substract(struct timeval *from, struct timeval *until) | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in 0b9026f, thanks for noticing.
There was a problem hiding this comment.
in french, the word is "soustraction"
Oh! And I've just learned that sous-chef means sub-chef!
| } \ | ||
| *okp = 1; \ | ||
| return h2o_timeval_subtract((from), (until)); \ | ||
| } \ |
There was a problem hiding this comment.
How about merging the two functions as int h2o_time_compute_##name(struct st_h2o_req_t *req, int64_t *delta_usec), and use the function from both logconf.c and durations.c?
Generally speaking, I think we should better refrain from introducing functions for just returning different types. Also, I think that the return value should be whether if the function succeeded.
There was a problem hiding this comment.
How about merging the two functions as int h2o_time_compute_##name(struct st_h2o_req_t *req, int64_t *delta_usec), and use the function from both logconf.c and durations.c?
The reason i didn't do that is that it seemed wasteful to combine the values into a usec just to split them again for the logconf.c case. I'll combine them into a single function.
There was a problem hiding this comment.
Thank you for the clarification.
I appreciate your concern about performance.
I think that in case we emit a log line it is highly likely that there are other stringifications will be performed as well (e.g. IP address, status code, content length, etc.), and that the impact of an extra multiply+divide would be negligible.
Or I am open to changing the signature to int h2o_time_compute_##name(struct st_h2o_req_t *req, struct timeval *delta_tv) if it seems more adequate to you.
| assert(ret.len < BUFSIZE); | ||
|
|
||
| return ret; | ||
| if (JEMALLOC_STATS) { |
There was a problem hiding this comment.
Could you please change if (JEMALLOC_STATS) { ... } to #if JEMALLOC_STATS ... #endif? The reason I suggested this is since there is not guarantee that a compiler strips non-reachable code (i.e. the block inside if (0) { }). In such case, reference to malloc_stats_print would cause a link-time error.
There was a problem hiding this comment.
Ah yes, i misread your suggestion, thank you for the reminder. Updating.
|
Thank you for the updates. Aside from the |
There's no guarantee that the compiler will actually compile the code out. An `#if` does guarantee that.
Fixed in 5764880. Thank you. |
|
Thank you for the updates! Merged to master. |
This commit adds duration and jemalloc statistics, both are optional:
jemalloc is enabled at compile time and the duration statistics are
enabled by a 'duration-stats' configuration keyword.
The commit also adds a total number of connections counter.
Duration statistics build on the existing per request duration stats
that are available for logging, each observation is stored in a
structure that allows computing quantiles. When the status page is
queried, values for the minimal, 25th, 50th, 75th and 99th percentile
are output.