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

Change default logger level from WARNING to ERROR #6612

Closed
mattab opened this Issue Nov 7, 2014 · 6 comments

Comments

Projects
None yet
3 participants
@mattab
Copy link
Member

mattab commented Nov 7, 2014

Currently WARN level logs are logged to the screen: https://github.com/piwik/piwik/blob/master/config/global.ini.php#L54-L61

This is causing problems, for example core:archive command errors with:

ERROR CoreConsole[2014-10-26 23:59:53] [7ece2] Got invalid
response from API request:
http://demo.piwik.org/index.php?module=API&method=CoreAdminHome.runScheduledTasks&format=csv&convertToUnicode=0&token_auth=trigger=archivephp.
Response was 'WARN ScheduledReports[2014-10-26 23:59:45] [59c46]
Preventing the same scheduled report from being sent again (report #25
for period "Week 13 October - 19 October 2014") WARN
ScheduledReports[2014-10-26 23:59:46] [59c46] Preventing the same
scheduled report from being sent again (report #3 for period "Week 13
October - 19 October 2014") 

Which then fails the archiving.

To prevent WARNINGS message from failing the archiving, I propose to change default logger level to ERROR so only ERRORS are logged to screen. (helps us detect bugs faster as users will report them or learn from them and fix it themselves)

I had a look at other Log::warning and could see those to change from warning to error:

(Other Log::warning look like they are real warning and not errors.)

@mattab mattab added the Task label Nov 7, 2014

@mattab mattab added this to the Piwik 2.9.0 milestone Nov 7, 2014

@mnapoli

This comment has been minimized.

Copy link
Contributor

mnapoli commented Nov 7, 2014

Here the warning messages are logged to the HTML output of the http://demo.piwik.org/index.php?module=API&method=CoreAdminHome.runScheduledTasks&format=csv&convertToUnicode=0&token_auth=trigger=archivephp HTTP request right?

Maybe there should be different logger backends depending on the environment? I.e. it doesn't make sense to log to the HTML output in an HTTP request context, however it makes totally sense to log to stdout when we are in a CLI context.

@diosmosis

This comment has been minimized.

Copy link
Member

diosmosis commented Nov 7, 2014

FYI, errors are always logged to the screen. The reason warnings are logged to the screen is because 'screen' is in log_writers setting in global.ini.php.

@mnapoli

This comment has been minimized.

Copy link
Contributor

mnapoli commented Nov 7, 2014

Yeah a fatal error/uncatched exception is a special case from logging really.

Warnings should be logged. And maybe we shouldn't take about "screen" because it's confusing. So what about setting different logging adapters/backends depending on the context?

  • web application -> log to the HTML output somewhere in the page
  • HTTP API -> don't log to the JSON it doesn't make sense
  • CLI -> log to stdout
  • logging to common backends like file of course

mattab added a commit that referenced this issue Nov 7, 2014

refs #6612 Set default logger level to ERROR so that only real errors…
… break the archiver (and not simple warnings)
@mattab

This comment has been minimized.

Copy link
Member Author

mattab commented Nov 7, 2014

I've changed default logger level to ERROR mostly because I needed to do something for 2.9.0 where currently the archiving breaks if any warning is emitted. I'll close issue but @mnapoli feel free to create another issue with your suggestions if you want

@mattab mattab closed this Nov 7, 2014

@diosmosis

This comment has been minimized.

Copy link
Member

diosmosis commented Nov 7, 2014

@mnapoli My comment wasn't an argument against your idea, just a note about the issue ;)

sgiehl added a commit that referenced this issue Nov 7, 2014

@mnapoli

This comment has been minimized.

Copy link
Contributor

mnapoli commented Nov 9, 2014

@diosmosis yes sorry if my wording wasn't the best ;)

I've opened a separate issue about the logger: #6622

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.