Log stream #103
Log stream #103
Conversation
lib.php
Outdated
| function tool_heartbeat_status_checks() { | ||
| return [new \tool_heartbeat\check\authcheck()]; | ||
| } | ||
|
|
||
| tool_heartbeat\logger::register_shutdown_handler(); |
There was a problem hiding this comment.
I don't think lib.php is always guaranteed to be loaded, I think this needs to be inside a callback such tool_heartbeat_after_config but this only landed in 3.8. We might not need older support, if we do there are other callbacks we can use
srdjan-catalyst
force-pushed
the
log-stream
branch
3 times, most recently
from
5f28a2f to
b8780f1
Compare
| // along with Moodle. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| /** | ||
| * MFA management class. |
lib.php
Outdated
| } | ||
|
|
||
| function tool_heartbeat_after_config() { | ||
| stream_wrapper_register('syslog', 'tool_heartbeat\wrapper\syslog') |
There was a problem hiding this comment.
can this be conditional so if it's not configured we don't add the wrapper
lib.php
Outdated
| } | ||
|
|
||
| function tool_heartbeat_after_config() { | ||
| stream_wrapper_register('syslog', 'tool_heartbeat\wrapper\syslog') |
There was a problem hiding this comment.
this doesn't even install:
Error
Exception - syntax error, unexpected 'throw' (T_THROW)
Debug info:
Error code: generalexceptionmessage
Stack trace:
line 35 of /admin/tool/heartbeat/lib.php: ParseError thrown
line 7833 of /lib/moodlelib.php: call to core_component::get_plugin_list_with_file()
line 1084 of /lib/setup.php: call to get_plugins_with_function()
line 651 of /config.php: call to require_once()
line 88 of /admin/index.php: call to require()
| $summary = get_string('logstreamerror', 'tool_heartbeat', "$logstream ".$e->getMessage()); | ||
| } | ||
| } else { | ||
| $status = result::WARNING; |
There was a problem hiding this comment.
I don't think we should check that config is in a certain state here, it is normal that you might not want to use this and it shouldn't be a warning.
| */ | ||
| public static function log(): void { | ||
| $defaultstream = self::default_log_stream(); | ||
| $statcallbacks = self::$statcallbacks; |
There was a problem hiding this comment.
It isn't very clear what all the lines from here to line 80 are doing. All of the $statcallbacks stuff seems very complicated, I believe it is all to allow injection for the unit tests? Is so could this just be tucked away into a getter?
| if ($fh = fopen($stream, 'a')) { | ||
| foreach ($lines as $l) { | ||
| if (fwrite($fh, "{$PAGE->pagetype} - $l") === false) { | ||
| throw new moodle_exception("tool_heartbeat\\logger::log_to_stream: Cannot write to $stream"); |
There was a problem hiding this comment.
I would like to avoid exceptions where we are adding instrumentation to a page and now the page has the possibility of failing due to the instrumentation. I think a debugging() call is a better option here
| $plugin->requires = 2012120311; // Deep support going back to 2.4. | ||
| $plugin->version = 2021122200; | ||
| $plugin->release = 2021122200; // Match release exactly to version. | ||
| $plugin->requires = 2018051718; // Deep support going back to 3.5. |
There was a problem hiding this comment.
This support cannot change
|
|
||
| if ($fh = fopen($stream, 'a')) { | ||
| foreach ($lines as $l) { | ||
| if (fwrite($fh, "{$PAGE->pagetype} - $l") === false) { |
There was a problem hiding this comment.
I'm not sure that $PAGE is always setup. Will comment in the wr we might want something else here
This is suboptimal, the same call may have already been made in request_shutdown()
srdjan-catalyst
force-pushed
the
log-stream
branch
from
b8780f1 to
7c5f2de
Compare
No description provided.