[RFC] Add extended stats by deweerdt · Pull Request #893 · h2o/h2o

deweerdt · 2016-05-05T16:05:46Z

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.

- 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 · 2016-05-14T18:24:46Z

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.

kazuho · 2016-05-16T13:03:50Z

+            h2o_iovec_t (*assemble_cb)(void *ctx);
+            void (*done_cb)(void *ctx);
+        } per_thread;
+    };


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 · 2016-05-16T13:53:11Z

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 · 2016-05-16T14:44:59Z

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

`init` and `per_thread` callbacks optional. Only `final` will be mandatory. - Remove GNUisms - Fix the doc to reflect the current implementation.

- Use predefined functions for the callsites where the status is known, avoiding one indirection level

kazuho · 2016-05-17T04:34:05Z

    pthread_mutex_t mutex;
-    h2o_iovec_t data;
    size_t num_remaining_threads;
+    h2o_globalconf_t *gconf;


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

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 · 2016-05-23T01:28:01Z

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

deweerdt · 2016-05-23T01:31:46Z

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

kazuho · 2016-05-23T08:13:57Z

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


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

kazuho · 2016-05-23T08:56:36Z

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

- 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 · 2016-05-23T22:53:07Z

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

kazuho · 2016-05-25T06:02:59Z

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 · 2016-05-25T17:01:54Z

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'?

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 · 2016-05-26T04:50:12Z

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

- Modify the json key naming: s/_/-/ and add a dot to denote categories

deweerdt · 2016-05-27T16:51:44Z

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

kazuho · 2016-05-30T03:16:17Z

+                                          " \"http1-errors.417\": %" PRIu64 ",\n"
+                                          " \"http1-errors.500\": %" PRIu64 ",\n"
+                                          " \"http1-errors.502\": %" PRIu64 ",\n"
+                                          " \"http1-errors.503\": %" PRIu64 ",\n"


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 ah yes, of course. That's fixed in a3b35d1

deweerdt · 2016-06-01T03:53:37Z

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

[RFC] Add extended stats clang-format applied to the updated files

kazuho · 2016-06-01T04:37:18Z

Thank you for your hard work! Merged to master.

deweerdt force-pushed the deweerdt/stats branch 3 times, most recently from ddb4076 to b267e10 Compare May 5, 2016 22:09

deweerdt force-pushed the deweerdt/stats branch from b267e10 to d0dafa0 Compare May 14, 2016 17:37

kazuho reviewed May 16, 2016
View reviewed changes

deweerdt added 2 commits May 16, 2016 13:47

- Simplify h2o_status_handler_t by removing the type, and making the

99b9acf

`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

7ee8e64

- Use predefined functions for the callsites where the status is known, avoiding one indirection level

kazuho reviewed May 17, 2016
View reviewed changes

kazuho reviewed May 23, 2016
View reviewed changes

Various renames

a24ec3d

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

- Rename 'errors' to 'events', since we're really tracking events here

bd158bc

- Modify the json key naming: s/_/-/ and add a dot to denote categories

kazuho reviewed May 30, 2016
View reviewed changes

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

s/http1-errors/status-errors/ in the json output as well

a3b35d1

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

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

Merge pull request #893 from pull/893

094ac82

[RFC] Add extended stats clang-format applied to the updated files

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

adjust docs (amend #893)

9b091c9

kazuho mentioned this pull request Jun 22, 2016

concurrent collection of request stats #961

Merged

Conversation

deweerdt commented May 5, 2016

Uh oh!

deweerdt commented May 14, 2016

Uh oh!

kazuho May 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazuho commented May 16, 2016

Uh oh!

deweerdt commented May 16, 2016

Uh oh!

kazuho May 17, 2016

Choose a reason for hiding this comment

Uh oh!

kazuho commented May 23, 2016

Uh oh!

deweerdt commented May 23, 2016

Uh oh!

kazuho May 23, 2016

Choose a reason for hiding this comment

Uh oh!

kazuho commented May 23, 2016

Uh oh!

deweerdt commented May 23, 2016

Uh oh!

kazuho commented May 25, 2016

Uh oh!

deweerdt commented May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazuho commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deweerdt commented May 27, 2016

Uh oh!

kazuho May 30, 2016

Choose a reason for hiding this comment

Uh oh!

deweerdt May 31, 2016

Choose a reason for hiding this comment

Uh oh!

deweerdt commented Jun 1, 2016

Uh oh!

kazuho commented Jun 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kazuho May 16, 2016 •

edited

Loading

deweerdt commented May 25, 2016 •

edited

Loading

kazuho commented May 26, 2016 •

edited

Loading