Skip to content

Conversation

@ethqunzhong
Copy link
Contributor

@ethqunzhong ethqunzhong commented Feb 10, 2022

Motivation

As default, Broker log level set by log4j2.yaml with pulsar.log.level=info, if developers want to update log level to debug, need to Modify log42j.yaml#pulsar.log.level=debug and then restart Broker service.

This PR support developers dynamic update Broker log level at runtime,It is very useful for online trouble shooting without having to stop Broker service and start it again.

Modifications

  • add admin cli (bin/pulsar-admin brokers update-logger-level --classname ${CLASSNAME} --level ${LEVEL})
  • add rest endpoints (/admin/v2/brokers/log4j/{classname}/{level})

Verifying this change

  • Make sure that the change passes the CI checks.
  • This change added tests and can be verified as follows:org.apache.pulsar.admin.cli.PulsarAdminToolTest#brokers
  • works as expected on product environment.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (yes)
  • The admin cli options: (yes)
  • Anything that affects deployment: (no)

Documentation

  • doc-not-needed

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 10, 2022
@ethqunzhong ethqunzhong force-pushed the dynamic_adjust_broker_log_level branch from 7d28dd8 to 041f15e Compare February 11, 2022 06:13
@ethqunzhong ethqunzhong requested a review from Jason918 February 11, 2022 06:28
additivity: true
AppenderRef:
- ref: "${sys:pulsar.log.appender}"
level: "${sys:pulsar.log.level}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be changed as we should keep default behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as default pulsar.log.level=info
i have do a test in product env as follow:
image

remove this item seems do not change default log4j behavior.

as answer in [stackoverflow:programmatically-change-log-level-in-log4j2 :

i have try all mentioned way to change logger level by programmatically way, it not work unless remove level: "${sys:pulsar.log.level}"

Have you ever had similar problems?

Copy link
Contributor

Choose a reason for hiding this comment

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

User may already have set pulsar.log.level=warn in their deployment. Will this PR change the settings?

Copy link
Contributor Author

@ethqunzhong ethqunzhong Feb 15, 2022

Choose a reason for hiding this comment

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

i have set pulsar.log.level=warn for test. but not what i expected. described as this issue #14298
update: 🤲
i have a test, result as follow:
as log4j2.yaml using way,
image
pulsar.log.level go into effect only set by ENV property PULSAR_LOG_LEVEL which set in pulsar_env.sh while service starting.
while service running, change pulsar.log.level in log4j2.yaml is not work, pulsar.log.level is unmodifiable now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User may already have set pulsar.log.level=warn in their deployment. Will this PR change the settings?

i have a test

if User set pulsar.log.level=warn in their deployment while starting service.

this PR will not change the settings , all loggers's level is warn, meanwhile , the PR featrue also works.

level: "${sys:pulsar.log.level}"

this line config is redundant and will disturb the programmatically way.

Copy link
Contributor

Choose a reason for hiding this comment

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

this PR will not change the settings

I am a little confused that if you removed this, how would user's previous setting (set by ENV property PULSAR_LOG_LEVEL in pulsar_env.sh) take effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have a test in standalone mode after removed this line.

  1. pulsar.log.level set by ENV property PULSAR_LOG_LEVEL in pulsar_env.sh to warn

image

2. then start pulsar service in standalone mode. the service log level is warn, worked as desired.

image

3. then run cmd `bin/pulsar-admin brokers update-logger-level --classname org.apache.zookeeper.server.FinalRequestProcessor --level debug` to validation feature in this PR. workd as desired.

image

so remove this line would not invalidate user's previous setting (set by ENV property PULSAR_LOG_LEVEL in pulsar_env.sh

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain from the code, why pulsar.log.level this system env could still take effect after we delete from log4j2.yaml? Did we pass this variable to log4j somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain from the code, why pulsar.log.level this system env could still take effect after we delete from log4j2.yaml? Did we pass this variable to log4j somewhere else?

+1, from the source code here https://github.com/apache/pulsar/blob/master/bin/pulsar#L299, I think we can't remove this line here.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Very interesting work.
I left some suggestions, PTAL

@ethqunzhong ethqunzhong force-pushed the dynamic_adjust_broker_log_level branch from 041f15e to ce21dc7 Compare February 18, 2022 09:10
@ethqunzhong ethqunzhong force-pushed the dynamic_adjust_broker_log_level branch from ce21dc7 to 0172610 Compare February 18, 2022 10:25
@wolfstudy
Copy link
Member

cc @eolivelli @Jason918 PTAL again thanks

@ethqunzhong
Copy link
Contributor Author

/pulsarbot run-failure-checks

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

IIRC log4j supports automatic reloading of configuration files.
Isn't it enough for you?

@ethqunzhong
Copy link
Contributor Author

IIRC log4j supports automatic reloading of configuration files. Isn't it enough for you?

yes, Log4j has the ability to automatically configure itself during initialization and reload config while config files changed.
but it is more convenient for developers update logger level by programmatically way.

additivity: true
AppenderRef:
- ref: "${sys:pulsar.log.appender}"
level: "${sys:pulsar.log.level}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain from the code, why pulsar.log.level this system env could still take effect after we delete from log4j2.yaml? Did we pass this variable to log4j somewhere else?

+1, from the source code here https://github.com/apache/pulsar/blob/master/bin/pulsar#L299, I think we can't remove this line here.

@ApiResponse(code = 403, message = "You don't have admin permission to update logger level."),
@ApiResponse(code = 500, message = "Internal server error")})
public void updateLoggerLevelDynamically(@Suspended final AsyncResponse asyncResponse,
@PathParam("classname") String classname,
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be the logger name here, not the class name. Usually, we will use the class name as the logger name, but not 100%

@ApiResponse(code = 204, message = "class logger level updated successfully"),
@ApiResponse(code = 403, message = "You don't have admin permission to update logger level."),
@ApiResponse(code = 500, message = "Internal server error")})
public void updateLoggerLevelDynamically(@Suspended final AsyncResponse asyncResponse,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it able to provide a get method to get the current applied log levels?

*
* @throws PulsarAdminException if update logger level failed.
*/
void updateLoggerLevel(String classname, String level) throws PulsarAdminException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
void updateLoggerLevel(String classname, String level) throws PulsarAdminException;
void updateLoggerLevel(String loggerName, String level) throws PulsarAdminException;

* Reset logger level dynamically in runtime asynchronously.
* @return CompletableFuture
*/
CompletableFuture<Void> updateLoggerLevelAsync(String classname, String level);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CompletableFuture<Void> updateLoggerLevelAsync(String classname, String level);
CompletableFuture<Void> updateLoggerLevelAsync(String loggerName, String level);

private class UpdateLoggerLevelCmd extends CliCommand {
@Parameter(names = {"-c", "--classname"}, description =
"The except class name,if set \"ROOT\" will take effect to rootLogger", required = true)
private String classname;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private String classname;
private String loggerName;


@Parameters(commandDescription = "Dynamic update logger level in runtime by classname.")
private class UpdateLoggerLevelCmd extends CliCommand {
@Parameter(names = {"-c", "--classname"}, description =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Parameter(names = {"-c", "--classname"}, description =
@Parameter(names = {"-l", "--logger-name"}, description =

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

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

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test release/4.0.10

Projects

None yet

Development

Successfully merging this pull request may close these issues.