Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config to turn off sending the logs to Loki #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

X-Coder264
Copy link

After configuring the logger in our Laravel app I've noticed that the test runs in the CI pipeline became a lot slower. Turns out that the package is always trying to ping Loki and since our Docker CI setup has no internet connectivity (on purpose) that slows down the tests as the handler waits until curl timeouts.

I've added a new config setting to address that so that we can disable this handler for the test environment.

@rrajkomar
Copy link
Contributor

First of all, thank you for your contribution.
That is indeed something that happens when the Loki server is unavailable and/or unreachable.
However I do believe that you can already bypass this by overrriding the curl options (see the curl_options setting which was added by @cstuder) with CURLOPT_TIMEOUT_MS set to 0.
Did you try this settting ? Please try and get back to me.
In the meantime I'll leave this PR opened.

@X-Coder264
Copy link
Author

https://www.php.net/manual/en/function.curl-setopt.php
https://curl.se/libcurl/c/CURLOPT_TIMEOUT_MS.html

Use 0 to wait indefinitely.
Default timeout is 0 (zero) which means it never times out during transfer.

This is the opposite of what we want to achieve. And even if it had worked I still wouldn't want the entire record formatting/processing and curl handle init part to run for no reason.

@rrajkomar
Copy link
Contributor

rrajkomar commented Feb 17, 2022 via email

@X-Coder264
Copy link
Author

In Laravel all config is done through PHP files which return an array. The logging config file is here -> https://github.com/laravel/laravel/blob/9.x/config/logging.php

There's no concept in Laravel of environment specific config files like Symfony has (e.g. prod/monolog.yaml and test/monolog.yaml). Theoretically I could write some ugly if statements and then merge or not merge the logger into the returned array, but nobody does that in the Laravel ecosystem as all log handlers that I've worked with have a config option with which you can turn them off via an env variable so that the returned array in that logging.php file does not depend on the environment.

For example, in the case of the Sentry Monolog handler you just set the DSN parameter to an empty string or null and instead of an actual HTTP transport which sends stuff to Sentry you get the NullTransport which does nothing -> https://github.com/getsentry/sentry-php/blob/3.3.7/src/Transport/DefaultTransportFactory.php#L61-L63

@@ -111,6 +113,7 @@ monolog:
'context' => [],
'labels' => [],
'client_name' => '',
'is_sending_enabled' => (bool) env('IS_LOKI_LOGGING_ENABLED', true),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me a while to get back to this.
Please rename the parameter (and the corresponding property) to enableSend.
Also there seem to be some conflict in your PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants