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.

Sign up for GitHub

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

Jump to bottom

Update java rules for parsing stacktraces to incude preceding ERROR messages #11

Closed
wants to merge 1 commit into from
Closed

Update java rules for parsing stacktraces to incude preceding ERROR messages #11

wants to merge 1 commit into from

Conversation

jcotado
Copy link

@jcotado jcotado commented Mar 28, 2017

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@jcotado
Copy link
Author

jcotado commented Mar 28, 2017 via email

@googlebot
Copy link

CLAs look good, thanks!

@qingling128
Copy link
Contributor

Hi @jcotado,

Thank you so much for suggesting this change! Seems like you are trying to add the error message to the combined log entry, which is a valid use case. Yet the format we are coding against here is kind of too specific. We will consider adding support for this use case. It probably would be implemented in a more generic way through a language-independent config option though.

Before we could provide something like that, an easy workaround is to just use in_tail plugin. Please refer to the doc here for a proper regex for your case.

@george-angel
Copy link

@qingling128 To clatify, this is fairly standard parsing rules for Java application stacktrace, but it doesn't fit your (internal?) use case?

Also regarding you advise to use in_tail plugin, we are ingesting all our logs from containers in a kubernetes cluster by mounting /var/log, its already using tail but the format is json. Not sure how we would apply the regex there.

@qingling128
Copy link
Contributor

@george-angel Yeah, the stacktrace parsing part is indeed standard. e.g.:

SomeException: foo
  at bar
Caused by: org.AnotherException
  at bar2
  at bar3

This is supported by the current implementation. Seems our unit test cases do not cover yet though. I will add some test case for this.

What I mentioned that was not very generic is to bundle the ERROR <some_error_message> line to the combined log entry. Since the ERROR ... line is not produced by the Java standard e.printStackTrace(). It's probably from a separate line in the code to print the error I guess? And that format can vary based on developers' preference. This is not Java specific, but should be applied to all languages. And we would want to make the format configurable by developers.

As to in_tail plugin, it might be a different story when it comes to kubernetes. kubernetes relies on a built-in fluentd-gcp image for logging. In older kubernetes versions the fluentd configs are built into the image. The latest version should support mounting the config under /etc/fluentd/config.d, but it's not fully GA yet.

Is the support for standard java stacktrace parsing (example above) enough for your use case? Or is it a must have to bundle the error message with the combined log entry? If so, do you mind helping us to understand the use case a bit more? This would help us prioritize feature requests like this.

@jcotado
Copy link
Author

jcotado commented Mar 30, 2017 via email

This was referenced Mar 30, 2017
Closed
Merged
@qingling128
Copy link
Contributor

@jcotado Thanks for the additional info! I've split this PR into two parts:

The part to enhance java unit test has been moved to this pull request: #13.
The part to bundle error message to combined log entry has been moved to an open issue: #12.

We'll prioritize this accordingly.

@qingling128
Copy link
Contributor

#13 is merged. Follow up for error message bundling would be tracked in #12.

Closing this PR for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jcotado @googlebot @qingling128 @george-angel