Skip to content
This repository was archived by the owner on Jan 6, 2023. It is now read-only.

Added support for logging in JSON format to a file. #20

Merged
merged 2 commits into from
Nov 29, 2021

Conversation

NickWemekamp
Copy link

@NickWemekamp NickWemekamp commented Aug 24, 2021

This merge request add supports for writing JSON logs from a WebLogic server to a file directly on the server.
It uses the EcsFormatter from Elastic.

@NickWemekamp NickWemekamp force-pushed the weblogic-json-logging branch from ddd4fc5 to f626774 Compare August 24, 2021 10:25
@ddsharpe
Copy link
Member

ddsharpe commented Nov 4, 2021

@NickWemekamp While we get some reviewers on this, I need you to follow the process outlined in https://github.com/oracle/weblogic-monitoring-exporter/blob/master/CONTRIBUTING.md. In short, you need sign the contributors agreement and "sign" your a commit that is part of this PR.

Copy link
Member

@ddsharpe ddsharpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to sign OCA and commit.

fh.setFormatter(new EcsFormatter());
logger.addHandler(fh);
} else {
System.out.println("WebLogic Elasticsearch Logging Exporter is disabled");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@russgold, @marinakog, I'm not as familiar with the logging exporter code base. I'm guessing that we are using System.out.println for logging from the exporter because the Java logging system is being used for messages bridged from WebLogic, correct? Is it possible to structure this so that we can still use logging for the exporter's messages?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this code base at all, actually. And Huiling is the one who was testing this exporter, not Marina.

@ddsharpe ddsharpe requested review from markxnelson and removed request for russgold and marinakog November 4, 2021 14:44
@markxnelson
Copy link
Member

Hi @NickWemekamp, I am reviewing and testing, will be in touch soon. Thanks for your PR!

@markxnelson
Copy link
Member

I got this built and tested it. I noticed that the output has a subset of fields (see below). I'd like to make it include all the fields by default and/or make it configurable.

{
  "@timestamp": "2021-11-15T18:25:01.237Z",
  "log.level": "Info",
  "message": "JMS shutdown is complete.",
  "ecs.version": "1.2.0",
  "process.thread.name": "[ACTIVE] ExecuteThread: '0' for queue: 'weblogic.kernel.Default (self-tuning)'",
  "log.logger": "JMS"
}

There's a few little nitpicks, like WebLogic has a capital L that i'd need to fix up.

I'd probably like to think a bit more about the config file format too. This seems to basically "switch" between two diff formats. I think I'd like to look at something more structured, like a yaml file or something. But that'd be a breaking change, and so i'd probably make that change in a v2.0.

I might also like to think about making the build produce an uberjar so it is easier to install.

Are you interested in contributing to such changes? If not, are you ok for me to grab what you have here and take it a little further?

@markxnelson markxnelson self-assigned this Nov 15, 2021
@NickWemekamp
Copy link
Author

Hi @markxnelson thanks for your review.

I still need permission for the contributors agreement but I'll prioritise that tommorow.
I didn't realise until now that WebLogic has two uppercases :)

The switch between the two diff formats and having to explicitly disable the elasticsearch output is indeed a bit awkward, I wanted my contribution to be a non breaking change as much as possible that is why I did it this way. Feel free to change it. The fat jar and additional fields are useful as well. I think it is more practical if you take the code and edit it further as you have a better idea about what the log format should look like.

If you want to include additional fields like 'diagnosticContextId' I think having a custom formatter that looks similar to ECSFormatter is a good way to go. It is very helpful for us if the EcsJsonSerializer.serializeMDC(builder, mdcSupplier.getMDC()); invocation is included as well in the formatter because we use that for distributed tracing (e.g. group all logs belonging belonging to the same HTTP request). Maybe prefix the additional fields with the labels key as per the Elastic Common Schema (ECS additional fields) but I can always customise this when sending the logs to Elasticsearch with Filebeat.

@markxnelson
Copy link
Member

hi @NickWemekamp sounds good - it'd be great if you are able to sign the OCA, i could pull what you have contributed into a branch and add a few bits and pieces, so you'll get the credit :)

@NickWemekamp
Copy link
Author

I signed the OCA

…required dependencies.

Signed-off-by: Nick Wemekamp <[email protected]>
@NickWemekamp
Copy link
Author

@markxnelson I've added the missing WebLogic fields and a fat jar maven build. We're planning on using the JSON formatter so if you can make a new release (with or without the config file changes) that would be great.

@markxnelson
Copy link
Member

Thanks @NickWemekamp - I am planning to do a release for you early next week (holidays here) something like a 2.0-alpha-1 or something, with your stuff in it, and then when I get the other work done, we'll move towards a 2.0.

@markxnelson markxnelson merged commit 742ea49 into oracle:master Nov 29, 2021
@markxnelson
Copy link
Member

Hi @NickWemekamp - I did a release for you -> https://github.com/oracle/weblogic-logging-exporter/releases/tag/2.0-alpha-1
I took out the uberjar for now, I need to do a little bit of work on our side before I distribute an uberjar with these new dependencies bundled. I will try to get through that process asap and do another release for you with that in it.

@markxnelson
Copy link
Member

ok - i did what i need to do, i am going to respin that same release with the uberjar in it too

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants