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
Closed

Change default logger level from WARNING to ERROR #6612

mattab opened this issue Nov 7, 2014 · 6 comments
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@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 Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. label Nov 7, 2014
@mattab mattab added this to the Piwik 2.9.0 milestone Nov 7, 2014
@mnapoli
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
Copy link
Member

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
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
… break the archiver (and not simple warnings)
@mattab
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 as completed Nov 7, 2014
@diosmosis
Copy link
Member

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

@mnapoli
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
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

3 participants