Optionally submit some debugging details once for each request #5
Conversation
logs_http.module
Outdated
| @@ -152,6 +146,27 @@ function logs_http_register_event(array $log_entry) { | |||
| // Remove empty values, to prevent errors in the indexing of the JSON. | |||
| $event = logs_http_array_remove_empty($event); | |||
|
|
|||
| if (variable_get('logs_http_debugging_mode', FALSE)) { | |||
| if (!isset($events['debugging'])) { | |||
There was a problem hiding this comment.
I think that since debugging requires custom data, maybe we should instead invoke a hook (e.g. logs_http_debug_extra_data)?
There was a problem hiding this comment.
Well, a hook could be added to allow other modules to add even more detail if they needed to. But the module itself should already do what it says if we add that option, I think. So, adding GET, POST and COOKIE information should be done by this module and after that a hook could allow others to alter that result.
|
Are you interested in moving this one forward? Is there anything I should be doing? |
logs_http.module
Outdated
| if (variable_get('logs_http_debugging_mode', FALSE)) { | ||
| if (!isset($events['debugging'])) { | ||
| $debugging_event = $event; | ||
| unset($debugging_event['type']); |
There was a problem hiding this comment.
sorry, lots of issue in the stack...
Why this unsets?
There was a problem hiding this comment.
If a single http request is producing more than one log entry - which is very likely to happen - then the extra data should only be logged once, because otherwise it would be redundant. And that single extra log entry should not contain the message and severity of the regular log line, that's why $event gets cloned into $debugging_event which receives the unset's and gets added to the $events array.
There was a problem hiding this comment.
preventing identical events already happens a few lines down -- https://github.com/Gizra/logs_http/pull/5/files#diff-922d6884cd537678aefd6dad2cc40076R171
There was a problem hiding this comment.
Not really. That very first call is adding two events. The first one is the debugging info and the second one is unified with the hashed key.
There was a problem hiding this comment.
sorry, I'm not sure I follow. Why don't we just do:
if (variable_get('logs_http_debugging_mode', FALSE)) {
$events['debug'] = array(
'GET' => $_GET,
'POST' => $_POST,
'COOKIE' => $_COOKIE,
);
}There was a problem hiding this comment.
what am I missing? Can you give an example of the output you are looking for?
There was a problem hiding this comment.
No, the debug Info should not be part of the event because each request can produce any number of events and we don't want to include the debug information into each of them as it will be the same content every single time. That's not useful.
We only want to create one extra event with the debugging information and that should be the first one in the series of events. And that'sexactly what the PR achieves.
There was a problem hiding this comment.
We only want to create one extra event with the debugging information and that should be the first one in the series of events
I disagree - when you are debug mode I think the debug should be per event.
I think this emphasises that instead of hardcoding the debug info we should probably provide a hook and drupal_alter() when in debug mode.
So for example I would add the debug per event (i.e. use the drupal_alter()) while you will probably use the hook to register a new event using logs_http_register_event()).
What do you say? :)
There was a problem hiding this comment.
Such a hook is a good idea for individual extra debugging requirement but that can already be done if a custom module implement hook_watchdog(). But this module should provide a useful default so that people can start using it without having to write a custom module.
So, our difference is whether the debugging information should be included once per request or any number of times per request. Remember, the arrays included can be really verbose and I fear it gets difficult to debug if you have all that information - which is always the same - included maybe dozens of times.
However, if you still think it should be done that way, why don't we leave that to the site owner and allow them to decide themselves by providing an option in the settings?
There was a problem hiding this comment.
But this module should provide a useful default so that people can start using it without having to write a custom module.
I agree, so what I think we need to do is:
- Add a hook
- Implement that hook and apply your logic inside
if (variable_get('http_logs_debug_enviorement', FALSE) {
|
I have just moved the debugging details into an alter hook as discussed before. |
See #4
I have implemented this in a way that the debugging details get only submitted once per HTTP request, so even if one request to Drupal generates multiple events, there will only be one single event with debugging details.
Also, complex data within the JSON command is possible according to this blog post https://www.loggly.com/blog/8-handy-tips-consider-logging-json but only if the same field (here: "debug_details") has not been used for strings before. I have tested this and can confirm that it works as described.