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 option to configure GCP logging extension's logging target #455

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

zanmagerl
Copy link
Contributor

@zanmagerl zanmagerl commented Jun 18, 2023

This resolves #453.

I introduced two new configuration options:

  • enableConsoleLogging: with this user can enable or disable default Quarkus console logging. By default, this is disabled so that we prevent duplicated logs.
  • logTarget: with this user can configure the target of the logs: stdour, stderr, or Google Cloud API. It follows a similar approach as in the official Google Java Logging library (we even use their enums to avoid having to add additional boilerplate code).

With this user has the option to configure logging targets as they wish. They can output logs to Cloud Logging API and still use Quarkus console logging or they disable console logging in case they do not need them or they want to avoid duplication of logs (if they host their app on Google Cloud Platform).

Testing: using Quarkus getting-started sample on local and on GCP environment.

* Enable or disable default Quarkus console logging.
*/
@ConfigItem
public boolean enableConsoleLogging;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This config property is not used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is used, inside LoggingBuildSteps, where we configure if we want to disable Quarkus console logging

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes but it is not used in runtime code so it seems useless, the logTarget is enough IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not useless because it disables console logging with which we avoid duplicated logs. This GCP extension is currently registering another log handler, which causes us to have 2 logging handlers and 2 log outputs. With this option, we have the option to disable this by default so that user does not have to disable them manually (but they still have the option to keep console logging if they want).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just understood what you did!

If someone want to disable console login, he can use the quarkus.log.console.enable property, I don't see the point of providing another config property that will set this one for him.

Copy link
Contributor Author

@zanmagerl zanmagerl Jun 28, 2023

Choose a reason for hiding this comment

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

Sure I just thought it would be nice to have this disabled by default, but ok. Maybe would be beneficial to put this somewhere in the docs so users are not confused if they get doubled logs :)

* Enable or disable default Quarkus console logging.
*/
@ConfigItem
public boolean enableConsoleLogging;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes but it is not used in runtime code so it seems useless, the logTarget is enough IMHO

Copy link
Collaborator

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@loicmathieu loicmathieu merged commit a19a0d2 into quarkiverse:main Jul 5, 2023
1 check passed
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.

Add option to write to stdout in Google Cloud Logging
2 participants