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 messageJson resolver, for use with StringMapMessage and its ilk #15

Merged
merged 4 commits into from
Oct 9, 2018

Conversation

mkedwards
Copy link

No description provided.

@mkedwards mkedwards changed the title Add message_json resolver, for use with StringMapMessage and its ilk Add messageJson resolver, for use with StringMapMessage and its ilk Oct 7, 2018
@vy
Copy link
Owner

vy commented Oct 7, 2018

Hey @mkedwards. Thanks so much for the patch! Actually, the idea of handling messages is a brilliant idea. Though I do have a few concerns:

  • Capturing ClassCastExceptions to fallback to ParametrizedMessage is not something I would prefer. I will think about if we can cover a wider range (all?) of Message implementations via type check at runtime.
  • You shouldn't create your own ObjectMapper, but rather used the one available in the TemplateResolverContext.

I will try to address these issues (hopefully) sometime this week.

@mkedwards
Copy link
Author

mkedwards commented Oct 8, 2018

I think MultiformatMessage is intended to be the interface for "messages that can serialize to JSON/XML/etc."; it's the thing that has the getFormats() and getFormattedMessage() methods on it. The intention was that, if you are configuring in the JSON message expansion, you probably are mostly using structured messages, so you want that to be the fast path. But maybe it's worth doing instanceof so that the fallback path is a branch rather than an exception catch? I'm not that familiar with the performance difference between the two in a current JVM.

Thanks for the heads-up about the ObjectMapper; will do.

@emschwar
Copy link
Contributor

emschwar commented Oct 8, 2018

@vy I think the commits I just added should address your concerns. I removed the additional ObjectMapper, and changed the exception handling to an instanceof check-- though please let me know if you prefer a different style there.

vy added a commit that referenced this pull request Oct 8, 2018
vy added a commit that referenced this pull request Oct 8, 2018
@vy
Copy link
Owner

vy commented Oct 8, 2018

@mkedwards, @emschwar, why don't we handle JSON emitting MultiformatMessages in MessageResolver rather than creating a new resolver? I have pushed my version to nexiahome-nexia branch. If you are ok with that, I can make a new release.

vy added a commit that referenced this pull request Oct 8, 2018
@mkedwards
Copy link
Author

Looks pretty good to me! I can think of ways to micro-optimize this but I'm not going to worry about that until I've got some better benchmarks for the use cases I'm concerned about, in which we're trying to be selective about which events we log, and avoid expensive operations (using log4j2 lambda-wrapped values and similar trickery) when we're not logging. (And memoize them when we are, in case of multiple appenders.)

@mkedwards
Copy link
Author

@vy, we've tested your version with our local MultiformatMessage subclass (similar to StructuredDataMessage but with some experimental interpolation / lazy-evaluation features) and the Kafka appender, and it works just fine.

We think we're going to be able to open-source our message class fairly soon, at which point it might be interesting to explore alternate layout/message interfaces that reduce serialization overhead. Maybe something along the lines of a visitor pattern, where the Layout passes a visitor to the Message and the Message does the tree-walk? Some people refer to that as a SAX-style API, and Jackson's "Streaming API" (JsonGenerator, https://fasterxml.github.io/jackson-core/javadoc/2.9/com/fasterxml/jackson/core/JsonGenerator.html) works that way.

Have you worked at all with log4j2 upstream? It seems like this system would benefit from some hard-core perfomance work, which generally involves some API refactoring. If we could get a team together, it would be satisfying to build a "fish tagging" system for Java distributed architectures, comparable to what I've helped build previously in a Javascript/Python/C++ world. (That system resembled log4j2's ContextDataInjector scheme, with the local context transferred as metadata on RESTful API requests and auto-captured by task closures for delegation across thread boundaries.) Netty-based services really need such a thing.

@vy vy merged commit 2cdebf8 into vy:master Oct 9, 2018
vy added a commit that referenced this pull request Oct 9, 2018
@vy
Copy link
Owner

vy commented Oct 9, 2018

Issue is fixed in v0.14 release. Moving the performance discussion to issue #17.

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.

3 participants