[RFC] Add extended stats #893

Merged
merged 8 commits into from Jun 1, 2016

Conversation

Projects
None yet
2 participants
@deweerdt
Member

deweerdt commented May 5, 2016

This PR aims at adding optional additional counters accessible via the status page. This would be enabled when status has the value EXTENDED.
I post this mainly as a conversation starter, in particular to discuss how the information is gathered. Following our offline discussion, my goal would be to add also http2 counters as well as duration counters in proxy.c.

Improvements to reported stats
- Add a structure to track internally generated errors
- Structure the stats a pluggable modules. New status reporting code can
  use the `h2o_config_register_status_handler` (for queries that need to
  access per thread data) or `h2o_config_register_simple_status_handler`
  (for queries that can be run in the request context) APIs in order to
  add new statuses
- requests statuses and error reporting is moved under
  `lib/handler/status/`
@deweerdt

This comment has been minimized.

Show comment
Hide comment
@deweerdt

deweerdt May 14, 2016

Member

Here's another attempt at the PR: i've added a registering mechanism for request handlers ('main', 'requests' and 'errors'), and split requests and errors to their own file. I've also removed the jemalloc bits: haven't figured out how to make the detection work in the static linking case, and it feels like it should be a different PR anyway.

Member

deweerdt commented May 14, 2016

Here's another attempt at the PR: i've added a registering mechanism for request handlers ('main', 'requests' and 'errors'), and split requests and errors to their own file. I've also removed the jemalloc bits: haven't figured out how to make the detection work in the static linking case, and it feels like it should be a different PR anyway.

include/h2o.h
+ h2o_iovec_t (*assemble_cb)(void *ctx);
+ void (*done_cb)(void *ctx);
+ } per_thread;
+ };

This comment has been minimized.

@kazuho

kazuho May 16, 2016

Member

How about having a single type that optionally defines per_thread_cb, instead of having two distinct types.

For example, we could define three callbacks: init, per_thread, final, with the first two being optional callbacks. final should either a) gather the information obtained by calls to per_thread (that updates the struct costructed by init), or b) simply return the information available in a global storage (in this case, init and per_thread can be set to NULL).

Also, you cannot allocate memory from req->pool in a different thread (than the request belongs to). Therefore, you should not pass src_req to alloc_context_cb (and considering the fact that the status handler is rarely called, I think it is not mandatory to use memory pools in the handler).

@kazuho

kazuho May 16, 2016

Member

How about having a single type that optionally defines per_thread_cb, instead of having two distinct types.

For example, we could define three callbacks: init, per_thread, final, with the first two being optional callbacks. final should either a) gather the information obtained by calls to per_thread (that updates the struct costructed by init), or b) simply return the information available in a global storage (in this case, init and per_thread can be set to NULL).

Also, you cannot allocate memory from req->pool in a different thread (than the request belongs to). Therefore, you should not pass src_req to alloc_context_cb (and considering the fact that the status handler is rarely called, I think it is not mandatory to use memory pools in the handler).

include/h2o.h
+ H2O_STATUS_HANDLER_PER_THREAD,
+};
+typedef h2o_iovec_t (*simple_status_handler_cb)(h2o_globalconf_t *gconf, h2o_mem_pool_t *pool);
+typedef struct h2o_status_handler {

This comment has been minimized.

@kazuho

kazuho May 16, 2016

Member

Please rename the struct to struct st_h2o_status_handler_t (since others have st_ prefix and _t suffix).

@kazuho

kazuho May 16, 2016

Member

Please rename the struct to struct st_h2o_status_handler_t (since others have st_ prefix and _t suffix).

lib/handler/status/errors.c
+}
+
+h2o_status_handler_t errors_status_handler = {
+ .type = H2O_STATUS_HANDLER_PER_THREAD,

This comment has been minimized.

@kazuho

kazuho May 16, 2016

Member

Please refrain from using GCC extensions, since it will make porting H2O difficult.

@kazuho

kazuho May 16, 2016

Member

Please refrain from using GCC extensions, since it will make porting H2O difficult.

include/h2o.h
+ /**
+ * counter for errors internally emitted by h2o
+ */
+ h2o_emitted_errors_t emitted_errors;

This comment has been minimized.

@kazuho

kazuho May 16, 2016

Member

Let's move HTTP/2 protocol errors inside h2o_context_t::http2. And we can simply access the error array for HTTP2 using an index, since HTTP/2 errors are defined starting from 1 in the spec.

@kazuho

kazuho May 16, 2016

Member

Let's move HTTP/2 protocol errors inside h2o_context_t::http2. And we can simply access the error array for HTTP2 using an index, since HTTP/2 errors are defined starting from 1 in the spec.

This comment has been minimized.

@kazuho

kazuho May 16, 2016

Member

Re logging of status codes.

My understanding is that the intention is not to log the status codes in general, but is to log the errors that originated within H2O. That means that the source of the error and the logic that updates the counter can be tightly coupled, and IMO we should better couple them instead of passing the number as a value and then using switch-case to determine the slot for two reasons: 1) speed 2) ease of maintenance.

There could be various ways to couple them, but my two cents go to the following approach.

  1. rename h2o_send_error to h2o_send_error_generic (or something)
  2. rewrite invocations of h2o_send_error taking constant status codes to h2o_send_errorXXX, where XXX is the status code.
  3. define all the h2o_send_errorXXX functions using macro expansion. The functions should increment the corresponding counters and then call h2o_send_error_generic

How does it sound?

@kazuho

kazuho May 16, 2016

Member

Re logging of status codes.

My understanding is that the intention is not to log the status codes in general, but is to log the errors that originated within H2O. That means that the source of the error and the logic that updates the counter can be tightly coupled, and IMO we should better couple them instead of passing the number as a value and then using switch-case to determine the slot for two reasons: 1) speed 2) ease of maintenance.

There could be various ways to couple them, but my two cents go to the following approach.

  1. rename h2o_send_error to h2o_send_error_generic (or something)
  2. rewrite invocations of h2o_send_error taking constant status codes to h2o_send_errorXXX, where XXX is the status code.
  3. define all the h2o_send_errorXXX functions using macro expansion. The functions should increment the corresponding counters and then call h2o_send_error_generic

How does it sound?

This comment has been minimized.

@deweerdt

deweerdt May 16, 2016

Member

I believe the second of the two new commits address what you suggested. I had to keep a few calls directly to h2o_send_error_generic, since sometimes the error isn't directly known in the call site.
Thanks!

@deweerdt

deweerdt May 16, 2016

Member

I believe the second of the two new commits address what you suggested. I had to keep a few calls directly to h2o_send_error_generic, since sometimes the error isn't directly known in the call site.
Thanks!

This comment has been minimized.

@kazuho

kazuho May 17, 2016

Member

Thank you for the changes. The commit looks good.

Some additional comments from me are as follows. How do they sound?

  1. instead of updating the counters in h2o_send_error_deferred, rename the function to h2o_send_error_generic_deferred that does not do the update, and instead modify the counters at the location that calls the deferred function
  2. with 1 being done, we can remove h2o_context_report_http1_error
  3. remove E_HTTP4xx or E_HTTP5xx now that we have all the error codes that updates the counter hard-coded
@kazuho

kazuho May 17, 2016

Member

Thank you for the changes. The commit looks good.

Some additional comments from me are as follows. How do they sound?

  1. instead of updating the counters in h2o_send_error_deferred, rename the function to h2o_send_error_generic_deferred that does not do the update, and instead modify the counters at the location that calls the deferred function
  2. with 1 being done, we can remove h2o_context_report_http1_error
  3. remove E_HTTP4xx or E_HTTP5xx now that we have all the error codes that updates the counter hard-coded
srcdoc/configure/status_directives.mt
@@ -19,6 +19,9 @@ EOT
<p>
Access to the handler should be <a href="configure/mruby.html#access-control">restricted</a>, considering the fact that the status includes the details of in-flight HTTP requests.
The example below uses <a href="configure/basic_auth.html">Basic authentication</a>.
+Displaying in-flight HTTP requests in the json query
+(<code>/path-to-handler/json</code>) can optionally be disabled by
+passing <code>?noreqs</code> as a parameter to the url.

This comment has been minimized.

@kazuho

kazuho May 16, 2016

Member

Should noreqs better be renamed to stats-only? I assume that is what you want.

@kazuho

kazuho May 16, 2016

Member

Should noreqs better be renamed to stats-only? I assume that is what you want.

This comment has been minimized.

@deweerdt

deweerdt May 16, 2016

Member

That's actually an oversight on my end, I've updated the docs to reflect the current implementation.

@deweerdt

deweerdt May 16, 2016

Member

That's actually an oversight on my end, I've updated the docs to reflect the current implementation.

This comment has been minimized.

@kazuho

kazuho May 17, 2016

Member

Thank you for the update.

I like the current approach (using a positive list). Left in-line comments regarding the details of the implementation.

@kazuho

kazuho May 17, 2016

Member

Thank you for the update.

I like the current approach (using a positive list). Left in-line comments regarding the details of the implementation.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho May 16, 2016

Member

Thank you for updating the PR.

It seems like we've made a big progress forward. Thank you very much.

I've left high-level design comments inline. Would you please consider them?

Member

kazuho commented May 16, 2016

Thank you for updating the PR.

It seems like we've made a big progress forward. Thank you very much.

I've left high-level design comments inline. Would you please consider them?

@deweerdt

This comment has been minimized.

Show comment
Hide comment
@deweerdt

deweerdt May 16, 2016

Member

Hello @kazuho, Thanks for taking the time to review the PR! I'll be addressing your comments shortly.

Member

deweerdt commented May 16, 2016

Hello @kazuho, Thanks for taking the time to review the PR! I'll be addressing your comments shortly.

deweerdt added some commits May 16, 2016

- Simplify `h2o_status_handler_t` by removing the type, and making the
  `init` and `per_thread` callbacks optional. Only `final` will be
  mandatory.
- Remove GNUisms
- Fix the doc to reflect the current implementation.
- Split errors in http1/http2
- Use predefined functions for the callsites where the status is known,
  avoiding one indirection level
lib/handler/status.c
size_t num_remaining_threads;
+ h2o_globalconf_t *gconf;

This comment has been minimized.

@kazuho

kazuho May 17, 2016

Member

I believe you do not need to retain a pointer to h2o_globalconf_t. It can be reached by accessing h2o_context_t::globalconf.

@kazuho

kazuho May 17, 2016

Member

I believe you do not need to retain a pointer to h2o_globalconf_t. It can be reached by accessing h2o_context_t::globalconf.

lib/handler/status.c
@@ -236,9 +234,47 @@ static int on_req_json(struct st_h2o_status_handler_t *self, h2o_req_t *req)
return 0;
}
+static h2o_iovec_t *build_status_list(const char *param, size_t len)

This comment has been minimized.

@kazuho

kazuho May 17, 2016

Member

How about calling h2o_contains_token within on_req_json for each of the status handlers, instead of building a list of selected names using this function?

Also, I would suggest using , as the separactor instead of using | (note that | is a reserved character in the URI syntax).

@kazuho

kazuho May 17, 2016

Member

How about calling h2o_contains_token within on_req_json for each of the status handlers, instead of building a list of selected names using this function?

Also, I would suggest using , as the separactor instead of using | (note that | is a reserved character in the URI syntax).

include/h2o.h
@@ -411,6 +417,41 @@ typedef struct st_h2o_mimemap_type_t {
} data;
} h2o_mimemap_type_t;
+/* errors emanating from h2o */
+enum {

This comment has been minimized.

@kazuho

kazuho May 17, 2016

Member

How about moving the definitions of H2O_HTTP2_ERROR_* in http2_internal.h here, instead of trying to define a separate enum?

@kazuho

kazuho May 17, 2016

Member

How about moving the definitions of H2O_HTTP2_ERROR_* in http2_internal.h here, instead of trying to define a separate enum?

include/h2o.h
+ /**
+ * counter for http2 errors internally emitted by h2o
+ */
+ unsigned long emitted_errors[E_HTTP2_MAX];

This comment has been minimized.

@kazuho

kazuho May 17, 2016

Member

Should we better use uint64_t?

@kazuho

kazuho May 17, 2016

Member

Should we better use uint64_t?

- Add a specialized `h2o_send_error_deferred` define so that we can
  update the counters directly from that function, instead of relying on
  `h2o_context_report_http1_error`
- Remove `h2o_context_report_http1_error` and `E_HTTP_{4,5,X}XX` enums:
  Every call side now uses a well known http return code.
- Simplify the `status_list` logic by calling `h2o_contains_token`. We
  also use `,` as a separator between the modules, rather than the
  reserved `|` (update docs and the test).
- Add a test case verifying the increment of the 404 counter
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho May 23, 2016

Member

@deweerdt Should I review the changes now or are you planning to push more?

Member

kazuho commented May 23, 2016

@deweerdt Should I review the changes now or are you planning to push more?

@deweerdt

This comment has been minimized.

Show comment
Hide comment
@deweerdt

deweerdt May 23, 2016

Member

@kazuho, I believe the changes are ready for review, thanks!

Member

deweerdt commented May 23, 2016

@kazuho, I believe the changes are ready for review, thanks!

lib/http1.c
@@ -153,7 +153,7 @@ static void entity_read_send_error(struct st_h2o_http1_conn_t *conn, int status,
set_timeout(conn, NULL, NULL);
h2o_socket_read_stop(conn->sock);
conn->req.http1_is_persistent = 0;
- h2o_send_error(&conn->req, status, reason, body, H2O_SEND_ERROR_HTTP1_CLOSE_CONNECTION);
+ h2o_send_error_generic(&conn->req, status, reason, body, H2O_SEND_ERROR_HTTP1_CLOSE_CONNECTION);

This comment has been minimized.

@kazuho

kazuho May 23, 2016

Member

Should we better invoke h2o_send_error_XXX in the callers of entity_read_send_error to collect the error codes?

@kazuho

kazuho May 23, 2016

Member

Should we better invoke h2o_send_error_XXX in the callers of entity_read_send_error to collect the error codes?

lib/http2/connection.c
@@ -859,6 +866,7 @@ static void on_read(h2o_socket_t *sock, const char *err)
h2o_http2_conn_t *conn = sock->data;
if (err != NULL) {
+ h2o_context_report_http2_error(conn->super.ctx, H2O_HTTP2_ERROR_OTHER);

This comment has been minimized.

@kazuho

kazuho May 23, 2016

Member

Do we need to log read / write errors?

If so, I would suggest separating the structures that count the HTTP/2 error codes and the internal errors (read / write errors), rather than adding a privately-defined slot to the list of HTTP/2 error codes.

Read / write errors could be logged more directly (for example by incrementing the counters directly like ++ctx->http2.events.read_closed). And also, the code and the structure for counting the HTTP/2 protocol-level errors could be renamed to better reflect the purpose.

@kazuho

kazuho May 23, 2016

Member

Do we need to log read / write errors?

If so, I would suggest separating the structures that count the HTTP/2 error codes and the internal errors (read / write errors), rather than adding a privately-defined slot to the list of HTTP/2 error codes.

Read / write errors could be logged more directly (for example by incrementing the counters directly like ++ctx->http2.events.read_closed). And also, the code and the structure for counting the HTTP/2 protocol-level errors could be renamed to better reflect the purpose.

lib/handler/status.c
+ void *p;
+ p = sh->init(&err);
+ if (!p) {
+ h2o_send_error_400(req, "Invalid Request", err.base, 0);

This comment has been minimized.

@kazuho

kazuho May 23, 2016

Member

Unless there is a specific reason, I believe we should not allow init callback to fail.

They should be implemented to always succeed (and that makes the code that calls the handlers simpler), and in case there is a chance of a init callback to fail, then the status handler should retain the error within the handler and emit an error message as part of the JSON when its final callback is called.

@kazuho

kazuho May 23, 2016

Member

Unless there is a specific reason, I believe we should not allow init callback to fail.

They should be implemented to always succeed (and that makes the code that calls the handlers simpler), and in case there is a chance of a init callback to fail, then the status handler should retain the error within the handler and emit an error message as part of the JSON when its final callback is called.

include/h2o.h
+
+enum {
+ /* http1 */
+ E_HTTP_400 = 0,

This comment has been minimized.

@kazuho

kazuho May 23, 2016

Member

Please change the names to H2O_STATUS_ERROR_XXX

@kazuho

kazuho May 23, 2016

Member

Please change the names to H2O_STATUS_ERROR_XXX

include/h2o.h
+ /**
+ * counter for http1 errors internally emitted by h2o
+ */
+ unsigned long emitted_errors[E_HTTP_MAX];

This comment has been minimized.

@kazuho

kazuho May 23, 2016

Member

The type should be changed to uint64_t as well.

@kazuho

kazuho May 23, 2016

Member

The type should be changed to uint64_t as well.

lib/handler/status/requests.c
+
+#include "h2o.h"
+
+struct requests_status_ctx {

This comment has been minimized.

@kazuho

kazuho May 23, 2016

Member

Please surround the name with st_XXX_t.

@kazuho

kazuho May 23, 2016

Member

Please surround the name with st_XXX_t.

lib/handler/status.c
@@ -31,71 +38,47 @@ struct st_h2o_status_context_t {
h2o_multithread_receiver_t receiver;
};
+struct status_ctx {

This comment has been minimized.

@kazuho

kazuho May 23, 2016

Member

Please surround the struct with st_XXX_t.

@kazuho

kazuho May 23, 2016

Member

Please surround the struct with st_XXX_t.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho May 23, 2016

Member

@deweerdt I've gone through the code and left my comments. Please update the code or let me know how you think. Thank you in advance.

Member

kazuho commented May 23, 2016

@deweerdt I've gone through the code and left my comments. Please update the code or let me know how you think. Thank you in advance.

stats: various small fixes
- simplify `st_h2o_status_handler_t` error handler in `init` removing
  error handling that level, and deferring to `final`.
- Remove H2O_HTTP2_ERROR_OTHER in favor of dedicated `read_closed` and
  `write_closed` errors.
- rename http1 errors under H2O_STATUS_ERROR_XXX
- replace the remaining callers to `h2o_send_error_generic` to use
  hardcoded errors
- append `DECL_` to the names of function declaring macros for clarity
@deweerdt

This comment has been minimized.

Show comment
Hide comment
@deweerdt

deweerdt May 23, 2016

Member

Hello @kazuho , I believe 07f3850 addresses all your remarks. Thanks!

Member

deweerdt commented May 23, 2016

Hello @kazuho , I believe 07f3850 addresses all your remarks. Thanks!

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho May 25, 2016

Member

Please do not include http1_ in the name, since the status codes sent over HTTP2 are also counted.

Member

kazuho commented on include/h2o.h in 07f3850 May 25, 2016

Please do not include http1_ in the name, since the status codes sent over HTTP2 are also counted.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho May 25, 2016

Member

Please use H2O_ prefix for a macro defined in a header file.

Member

kazuho commented on include/h2o.h in 07f3850 May 25, 2016

Please use H2O_ prefix for a macro defined in a header file.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho May 25, 2016

Member

Please change the names of the properties to match the ones added to h2o_context_t.

Please change the names of the properties to match the ones added to h2o_context_t.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho May 25, 2016

Member

I would appreciate it if the names of the errors were also updated to match those defined in h2o_context_t and errors_status_ctx_t. (This is not a MUST; we could leave them as-is if it has already become difficult for you to change the names).

I would appreciate it if the names of the errors were also updated to match those defined in h2o_context_t and errors_status_ctx_t. (This is not a MUST; we could leave them as-is if it has already become difficult for you to change the names).

This comment has been minimized.

Show comment
Hide comment
@deweerdt

deweerdt May 25, 2016

Member

Do you mean have the json member look like h2o-http2-error-flow-control and h2o-status-error-400 ? That sounds good to me.

Member

deweerdt replied May 25, 2016

Do you mean have the json member look like h2o-http2-error-flow-control and h2o-status-error-400 ? That sounds good to me.

This comment has been minimized.

Show comment
Hide comment
@deweerdt

deweerdt May 25, 2016

Member

Alternatively, we could have the json object look like something like:

 "h2o-http2-error" : {
  "flow-control": 2,
  "compression": 3,
 ...
},
"h2o-status-error": {
  "400": 0,
  "403", 1,
  ...
},

That would be less verbose in terms of json, what do you think?

Member

deweerdt replied May 25, 2016

Alternatively, we could have the json object look like something like:

 "h2o-http2-error" : {
  "flow-control": 2,
  "compression": 3,
 ...
},
"h2o-status-error": {
  "400": 0,
  "403", 1,
  ...
},

That would be less verbose in terms of json, what do you think?

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho May 25, 2016

Member

Thank you for the updates. All we have left are some naming issues. Once they get updated I will merge the PR (or if you want me to change the names please let me know).

PS. Regarding the names, it might be better to rename the "error" status handler to "events" status handler considering the fact that it can be used to collect numbers of any kind of events (and that italready collects the number of connection closes).

Member

kazuho commented May 25, 2016

Thank you for the updates. All we have left are some naming issues. Once they get updated I will merge the PR (or if you want me to change the names please let me know).

PS. Regarding the names, it might be better to rename the "error" status handler to "events" status handler considering the fact that it can be used to collect numbers of any kind of events (and that italready collects the number of connection closes).

@deweerdt

This comment has been minimized.

Show comment
Hide comment
@deweerdt

deweerdt May 25, 2016

Member

Thank you for the updates. All we have left are some naming issues. Once they get updated I will merge the PR (or if you want me to change the names please let me know).

I'll update the code, thanks. I'll await your decision on the json formatting and the naming of the handler below before updating the PR.

PS. Regarding the names, it might be better to rename the "error" status handler to "events" status handler considering the fact that it can be used to collect numbers of any kind of events (and that italready collects the number of connection closes).

My initial idea was to mirror what would be found in the error log, but you're right that collecting read and write closes are better described by being events than errors. Another argument in favor of 'events' is that those aren't really errors that are actionable by the administrator, they are just useful to monitor because a sudden change in rates might mean that an action is required. How about 'networking_events'?

Member

deweerdt commented May 25, 2016

Thank you for the updates. All we have left are some naming issues. Once they get updated I will merge the PR (or if you want me to change the names please let me know).

I'll update the code, thanks. I'll await your decision on the json formatting and the naming of the handler below before updating the PR.

PS. Regarding the names, it might be better to rename the "error" status handler to "events" status handler considering the fact that it can be used to collect numbers of any kind of events (and that italready collects the number of connection closes).

My initial idea was to mirror what would be found in the error log, but you're right that collecting read and write closes are better described by being events than errors. Another argument in favor of 'events' is that those aren't really errors that are actionable by the administrator, they are just useful to monitor because a sudden change in rates might mean that an action is required. How about 'networking_events'?

Various renames
s/http1_status_errors/emitted_error_status/
s/DECL_H2O_SEND_ERROR_XXX/H2O_SEND_ERROR_XXX/
s/errors_status_ctx/st_errors_status_ctx_t/
rename aggregated errors to something that matches the data being
aggregated
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho May 26, 2016

Member

Do you mean have the json member look like h2o-http2-error-flow-control and h2o-status-error-400?
Alternatively, we could have the json object look like something like:

I do not have a strong preference, but using the former with h2o- omitted (e.g. http2-error-flow-control) might be better. The reason is because it is easier to handle non-structured name-values pairs (e.g. when converting to CSV), while in case you have the power to handle structured JSON it is likely that you can easily extract some fields with certain prefix.

PS. I'd also suggest using . to for denoting category and use - to connect the words that consist a single name (example: http2-error.flow-control).

My initial idea was to mirror what would be found in the error log, but you're right that collecting read and write closes are better described by being events than errors. Another argument in favor of 'events' is that those aren't really errors that are actionable by the administrator, they are just useful to monitor because a sudden change in rates might mean that an action is required.

Agreed.

How about 'networking_events'?

I'd suggest omitting networking, since we might want to enhance the handler to count non-networking events (e.g. number of times a file-based mmap is issued).

Member

kazuho commented May 26, 2016

Do you mean have the json member look like h2o-http2-error-flow-control and h2o-status-error-400?
Alternatively, we could have the json object look like something like:

I do not have a strong preference, but using the former with h2o- omitted (e.g. http2-error-flow-control) might be better. The reason is because it is easier to handle non-structured name-values pairs (e.g. when converting to CSV), while in case you have the power to handle structured JSON it is likely that you can easily extract some fields with certain prefix.

PS. I'd also suggest using . to for denoting category and use - to connect the words that consist a single name (example: http2-error.flow-control).

My initial idea was to mirror what would be found in the error log, but you're right that collecting read and write closes are better described by being events than errors. Another argument in favor of 'events' is that those aren't really errors that are actionable by the administrator, they are just useful to monitor because a sudden change in rates might mean that an action is required.

Agreed.

How about 'networking_events'?

I'd suggest omitting networking, since we might want to enhance the handler to count non-networking events (e.g. number of times a file-based mmap is issued).

- Rename 'errors' to 'events', since we're really tracking events here
- Modify the json key naming: s/_/-/ and add a dot to denote categories
@deweerdt

This comment has been minimized.

Show comment
Hide comment
@deweerdt

deweerdt May 27, 2016

Member

@kazuho , i believe bd158bc addresses everything we've discussed so far.

Member

deweerdt commented May 27, 2016

@kazuho , i believe bd158bc addresses everything we've discussed so far.

lib/handler/status/events.c
+ " \"http1-errors.417\": %" PRIu64 ",\n"
+ " \"http1-errors.500\": %" PRIu64 ",\n"
+ " \"http1-errors.502\": %" PRIu64 ",\n"
+ " \"http1-errors.503\": %" PRIu64 ",\n"

This comment has been minimized.

@kazuho

kazuho May 30, 2016

Member

Please rename the errors to status-errors, as discussed in #893 (comment).

I believe that is the only change required to getting this merged. Thank you!

@kazuho

kazuho May 30, 2016

Member

Please rename the errors to status-errors, as discussed in #893 (comment).

I believe that is the only change required to getting this merged. Thank you!

This comment has been minimized.

@deweerdt

deweerdt May 31, 2016

Member

@kazuho ah yes, of course. That's fixed in a3b35d1

@deweerdt

deweerdt May 31, 2016

Member

@kazuho ah yes, of course. That's fixed in a3b35d1

@kazuho kazuho added this to the v2.1 milestone May 31, 2016

@deweerdt

This comment has been minimized.

Show comment
Hide comment
@deweerdt

deweerdt Jun 1, 2016

Member

@kazuho, a3b35d1 has the last batch of renames. Thanks!

Member

deweerdt commented Jun 1, 2016

@kazuho, a3b35d1 has the last batch of renames. Thanks!

@kazuho kazuho merged commit a3b35d1 into h2o:master Jun 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

kazuho added a commit that referenced this pull request Jun 1, 2016

Merge pull request #893 from pull/893
[RFC] Add extended stats

clang-format applied to the updated files

kazuho added a commit that referenced this pull request Jun 1, 2016

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Jun 1, 2016

Member

Thank you for your hard work! Merged to master.

Member

kazuho commented Jun 1, 2016

Thank you for your hard work! Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment