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 log darkening factor #1899

Merged
merged 2 commits into from
Apr 9, 2019
Merged

Add log darkening factor #1899

merged 2 commits into from
Apr 9, 2019

Conversation

dmlloyd
Copy link
Member

@dmlloyd dmlloyd commented Apr 5, 2019

Configure via quarkus.log.console.darken=<factor> where the factor should be a small integer (2 seems to work well for bright backgrounds).

@dmlloyd dmlloyd requested a review from cescoffier April 5, 2019 18:00
@cescoffier cescoffier self-assigned this Apr 7, 2019
@cescoffier
Copy link
Member

@dmlloyd I will try tomorrow. Wondering if we can use an environment variable for quarkus.log.console.darken (QUARKUS_LOG_CONSOLE_DARKEN if we follow the MP config rules).

Let me explain why:
One of the issue with this approach is the "in-container" usage. I don't really want to have my docker file using the -Dquarkus.log.console.darken=2 neither setting this in the application.config. However, I can use --env or even a global env file (env-file) in my docker run command.

@dmlloyd
Copy link
Member Author

dmlloyd commented Apr 8, 2019

@dmlloyd I will try tomorrow. Wondering if we can use an environment variable for quarkus.log.console.darken (QUARKUS_LOG_CONSOLE_DARKEN if we follow the MP config rules).

Yes, that should work. No, I take that back, I'm not sure it will work. It would definitely work to override an existing value, but I think you'd have to test it otherwise.

@cescoffier
Copy link
Member

cescoffier commented Apr 9, 2019

Unfortunately, it does not seem to work:

getting-started (zsh) 2019-04-09 09-27-54

The background / color are used are "solarized light" (it's what work well on stage)

@dmlloyd
Copy link
Member Author

dmlloyd commented Apr 9, 2019

I think I can do a one-off fix for this property. I have an idea...

@dmlloyd
Copy link
Member Author

dmlloyd commented Apr 9, 2019

OK with that change you are now able to give the QUARKUS_LOG_CONSOLE_DARKEN environment variable with no other configuration present and it will override the configuration.

@cescoffier
Copy link
Member

Thanks @dmlloyd. I will try that tonight (MTZ).

@gsmet
Copy link
Member

gsmet commented Apr 9, 2019

@cescoffier maybe let's try to get that one in 0.13.1?

@gsmet gsmet added this to the 0.13.1 milestone Apr 9, 2019
@cescoffier
Copy link
Member

Just tested.

It's not perfect (basically it's monochrome), but readable enough. Let's merge it like this, and improve it later with colors.

2  mvn-or-mvnw compile quarkus:dev (java) 2019-04-09 18-14-52

@cescoffier cescoffier merged commit 9547441 into quarkusio:master Apr 9, 2019
@dmlloyd dmlloyd deleted the darken-logs branch April 9, 2019 16:36
@dmlloyd
Copy link
Member Author

dmlloyd commented Apr 9, 2019

Great! Maybe try with 1 instead of 2 then?

@cescoffier
Copy link
Member

I've tried 1, 2 and 3... no much changes

@dmlloyd
Copy link
Member Author

dmlloyd commented Apr 9, 2019

Ah, you probably have a 256-color terminal. Not much else to do in that case unfortunately!

@dmlloyd
Copy link
Member Author

dmlloyd commented Apr 9, 2019

Actually I might be able to use a smoother color increment for 256 mode. Will mess around with it a bit.

@emmanuelbernard
Copy link
Member

@dmlloyd Was the binary mode of ligh/dark not working that you went for the darkening factor?
I would say that for the layman user that's a bit confusing this factor ratio.

Also no doc, I'll open an issue for it.

@dmlloyd
Copy link
Member Author

dmlloyd commented Apr 13, 2019

This fix is only meant as a short term workaround for Clement to get through his presentation. I'll come around to fixing it up once logmanager is upstreamed, which I've been steadily working on but has been a slightly lower priority.

I tried making it binary but it seemed that different background colors need different darkening or brightening factors. So I settled on the factor in the end.

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.

4 participants