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

netsvc: [IMP] add json logging #23417

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@blaggacao
Copy link
Contributor

commented Mar 1, 2018

Description of the issue/feature this PR addresses:
Syslog-like formatted logs are extremly difficult and unreliable to parse

Current behavior before PR:
A multiline tracback would get sliced per line in normal log aggregators

Desired behavior after PR is merged:
A multiline traceback is cleanly catched as one single data field, which can be easily alerted upon.
The error's module name and class name are exposed in a parse-friendly way, so that
statistics can be done and alerts triggered according to the reported error.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

Output logs in json format when doing log streaming.
This eliminates the need for any log parsers to catch the errors
across a potential multiline traceback (which is rather complicated or even impossible)

Provide structured and well parsable data by a log forwarder / collector.

netsvc: [IMP] add json logging
Output logs in json format when doing log streaming.
This eliminates the need for any log parsers to catch the errors
across a potential multiline traceback (which is rather complicated or even impossible)

Provide structured and well parsable data by a log forwarder / collector.
@blaggacao

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2018

Feedback-Ticket: 1820007

def formatException(self, exc_info):
result = super(DBFormatter, self).formatException(exc_info)
# Also provide the error's class & module name
return '<<<< ' + exc_info[0].__module__ + '.' + exc_info[0].__name__ + ' >>>>\n' + result

This comment has been minimized.

Copy link
@blaggacao

blaggacao Mar 1, 2018

Author Contributor

Maybe this it to much of a braking change, but I wanted to hear your opinion.
I think it will help reading the log stream during development...

@blaggacao

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2018

@@ -16,6 +16,11 @@
from . import sql_db
from . import tools

try:
from pythonjsonlogger import jsonlogger
except:

This comment has been minimized.

Copy link
@Elkasitu

Elkasitu Mar 2, 2018

Contributor

Never do bare except clauses, bad practice... try except ImportError

Furthermore, why the extra dependency? Can't we import the standard json lib and then make the JSONFormatter class do all the work?

This comment has been minimized.

Copy link
@blaggacao

blaggacao Mar 2, 2018

Author Contributor

Ok, thank you for helping me improve my code style! It actually is a full blown pep8 violation... I'll correct it.

I was thinking about just copying the whole file from upstream, it's one single file of about 150 LOC... But thing is I personally trust more in the experience of intricate use cases covered by a battle tested upstream library..

Another nice side effect of using a library: We don't need to introduce a configuration flag, so it will be used only by most advanced users by simply providing the library. What's your opinion on that?

This comment has been minimized.

Copy link
@blaggacao

blaggacao Mar 2, 2018

Author Contributor

Thinking about a config option would as well be interesting, please let me know what you prefer... I have no problem in refactoring...

blaggacao added some commits Mar 2, 2018

@Elkasitu

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

Hi @blaggacao

Thank you for your contribution, but unfortunately this change is not the right solution for the problem you're trying to solve, and we know this because we parse logs for the runbot, odoo.sh, among other things.

On top of this, pythonjsonlogger is an extra third-party dependency that has no guarantee of being maintained at the same pace as Odoo, not to mention that it seems inactive since Aug 2017.

Perhaps a better idea would be to have some sort of hook to which the server posts status information in a computer-readable format (think of GitHub webhooks), as always, PRs welcome as long as they make sense and are not too intrusive :-)

Have a nice day.

@Elkasitu Elkasitu closed this Mar 7, 2018

@blaggacao

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

Hi @Elkasitu I understand, I'll probably open another PR to propose an alternative implementation not using an external library, and with a config flag. To see if it probably can convince you... 😄

As to the hook you propose, I associate that with metrics instrumenting like in /metrics for a Prometheus scraper... I indeed thought of that, but on the other hand, what useful information to expose?

It would be interesting to prepare a patch which puts some tracer wrappers around some methods and exposes their CPU time in real time through an instrumented /metrics endpoint.

Question: As to the log parsing, can you share (or think of) a log parser which does parse streaming logs and does not fail on a multi-line traceback?
Of course I could be tailing the written logs, but then it's just another step in the chain... with SPOFs (log rotation tooling and the very log parser!).

Disclaimer: I use fluent-bit to forward the logs to a EFK (F for fluent instead of logstash). It is very low on memory, somewhat beta, but a very promising forwarder...

Without intent to sound bold, I'm not entirely sure, if JSON logging in general, it's the wrong solution for the problem I try to solve, though... 😉

The ultimate problem definition: eliminate SPOFs and reduce plumbing.

@blaggacao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

@Elkasitu Adrian, would you agree to guide me a little to tackle this and underlying issues of script-ability?

I have now the case where:

  • JSON logging would simplify my life with a EFK istack (F for fluent-bit not fluentd which is CNCF incubated!)
  • An instrumented Prometheus (also CNCF incubated!) endpoint would help tracing some specific methods in production (related with ticket: 1808042)

@blaggacao blaggacao deleted the blaggacao:10-add-optional-json-looging branch Aug 10, 2018

@blaggacao blaggacao referenced this pull request Oct 15, 2018

Closed

Improve logging #4

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.