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

Send all the Diagnostics logs to the stdout, instead of local files #16941

Merged

Conversation

buraksezer
Copy link
Contributor

This PR implements #16844. Now we can send all the diagnostics logs to the stdout by using the following:

java -jar -Dhazelcast.diagnostics.enabled=true -Dhazelcast.diagnostics.stdout=true hazelcast/target/hazelcast-4.1-SNAPSHOT.jar

Could you please review my PR?

@devOpsHazelcast
Copy link
Collaborator

devOpsHazelcast commented Apr 29, 2020

CLA assistant check
All committers have signed the CLA.

@pveentjer
Copy link
Contributor

Could you add a line of logging:

if file based logging enabled, write the path of the file
if stdout based logging enabled, write that it is enabled.

This will make debugging easier.

Apart from that it looks good to go. No major remarks.

@buraksezer
Copy link
Contributor Author

I fixed the naming problem and added more log lines about diagnostics log output. Could you review again?

@buraksezer buraksezer requested a review from pveentjer May 1, 2020 11:22
@hazelcast hazelcast deleted a comment from devOpsHazelcast May 4, 2020
@mmedenjak
Copy link
Contributor

run-lab-run

@mmedenjak mmedenjak closed this May 4, 2020
@mmedenjak mmedenjak reopened this May 4, 2020
@mmedenjak
Copy link
Contributor

Sorry, wrong button.

@mmedenjak mmedenjak requested a review from blazember May 4, 2020 07:48
@mmedenjak mmedenjak added Source: Community PR or issue was opened by a community user Type: Enhancement labels May 4, 2020
@mmedenjak mmedenjak added this to the 4.1 milestone May 4, 2020
@pveentjer
Copy link
Contributor

Minor logging improvement request. Otherwise good to go.

@tkountis tkountis self-requested a review May 5, 2020 11:07
Copy link
Contributor

@tkountis tkountis left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but wondering whether we want to keep escaping the more natural and dev friendly logger approach. Dev & Op teams are accustomed by now to having a logger and its configuration.

@buraksezer
Copy link
Contributor Author

Hi team,

Is there any update on this topic?

@mmedenjak mmedenjak removed the request for review from blazember July 2, 2020 12:19
@mmedenjak mmedenjak merged commit 0dfbb59 into hazelcast:master Jul 2, 2020
@mmedenjak
Copy link
Contributor

mmedenjak commented Jul 2, 2020

Sorry for the long wait @buraksezer and thank you for the contribution (cc @Holmistr ). I must admit I was delaying merging it because I wanted to take a closer look.

Regarding @tkountis 's comment, I agree we could switch the whole diagnostics to use a logger, then users could seamlessly switch between file, stdout, network and more outputs. I'll create an issue as I see it as a breaking change for the future.

Again, thank you for your contribution and hope to see you around soon.

@rdasilva-fispan
Copy link

Is this configuration working on 4.2?
I just tried it but I still see logs going to a log file. If something else has improved, can someone please point me to the docs as I don't see anything here?

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

6 participants