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

[improve][misc] Upgrade log4j2 to 2.23.1 #22327

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

nodece
Copy link
Member

@nodece nodece commented Mar 22, 2024

Motivation

Keep up-to-date, and then upgrade the slf4j to 2.x.

Release notes: https://logging.apache.org/log4j/2.x/release-notes.html

Modifications

  • Upgrade log4j2 from 2.18.0 to 2.23.1
  • Configuration.status from INFO to ERROR in the log4j2.yaml
    • Because Configuration.packages has been deprecated, the log4j2 prints the warn log, this breaks our test.
    • The simpleclient_log4j2 is no longer updated and doesn't provide Log4j2Plugins.dat, we can only suppress this warn log.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 22, 2024
@lhotari
Copy link
Member

lhotari commented Mar 22, 2024

Please update the LICENSE files too

@nodece
Copy link
Member Author

nodece commented Mar 22, 2024

/pulsarbot rerun-failure-checks

@nodece
Copy link
Member Author

nodece commented Mar 22, 2024

The CLI fails.

2024-03-22T13:16:23.693795743Z main WARN The use of package scanning to locate plugins is deprecated and will be removed in a future release

"packages" has been deprecated, please see apache/logging-log4j2@2fc1f32

packages: io.prometheus.client.log4j2

@lhotari Do you have any suggestions?

@lhotari
Copy link
Member

lhotari commented Mar 22, 2024

It seems that some packages use classes that have been deprecated by log4j2.

2024-03-22T13:16:23.693795743Z main WARN The use of package scanning to locate plugins is deprecated and will be removed in a future release

io.prometheus.client.log4j2 breaks the CLI test.

@lhotari Do you have any suggestions?

Perhaps we need to upgrade the Prometheus Java client to a recent version?
https://github.com/prometheus/client_java

@asafm any suggestions?

@lhotari
Copy link
Member

lhotari commented Mar 22, 2024

It seems that I misunderstood your previous comment. I can see that the line which causes the warning is in the log4j2.yaml file,

packages: io.prometheus.client.log4j2

@lhotari
Copy link
Member

lhotari commented Mar 22, 2024

Looks like it is used for exposing counters of log entries: https://github.com/prometheus/client_java/blob/simpleclient/simpleclient_log4j2/src/main/java/io/prometheus/client/log4j2/InstrumentedAppender.java

pulsar/conf/log4j2.yaml

Lines 85 to 86 in 70c4003

Prometheus:
name: Prometheus

pulsar/conf/log4j2.yaml

Lines 140 to 141 in 70c4003

- ref: Prometheus
level: info

@nodece
Copy link
Member Author

nodece commented Mar 22, 2024

"package" has been deprecated, and Prometheus SDK requires a new implementation for logging the metric, please see https://logging.apache.org/log4j/2.x/manual/plugins.html.

Perhaps we need to upgrade the Prometheus Java client to a recent version?

Prometheus [email protected] is no longer updated, we need to migrate to the new SDK 1.1.0 from 0.16, but 1.1.0 doesn't include the log4j2 plugin, and we are introducing the otel to the Pulsar.

#2735 introduces simpleclient_log4j2, do we really need this?

I need some help here.

@nodece
Copy link
Member Author

nodece commented Mar 26, 2024

Any updates?

@asafm
Copy link
Contributor

asafm commented Mar 28, 2024

Ok guys, there was a lot of information thrown around here, and for me most of it was new.
I'll try to summarize it all here:

  1. Log4j2 supports plugins. allowing you to extend Log4j. In our example, it allows anyone to add a new type of Appender.
  2. Prometheus Simple client has a dependency which we included in Pulsar:
   <dependency>
      <groupId>io.prometheus</groupId>
      <artifactId>simpleclient_log4j2</artifactId>
    </dependency>

The dependency includes a Log4j2 plugin, which adds a new Prometheus type Appender. As @lhotari noted it is:

@Plugin(name = "Prometheus", category = "Core", elementType = "appender")
public final class InstrumentedAppender extends AbstractAppender {

This Appender receives the logs (log events) and exposes metrics about them - how many logs per type, etc. In effect, it instruments Log4j2.

  1. The Prometheus type appender is used as you noted in the log4j2.yaml under Appenders
    Prometheus:
      name: Prometheus

and then referenced to "forward" the logs to that appender so they can be counted.

    # Default root logger configuration
    Root:
      level: "${sys:pulsar.log.root.level}"
      additivity: true
      AppenderRef:
        - ref: "${sys:pulsar.log.appender}"
          level: "${sys:pulsar.log.level}"
        - ref: Prometheus
          level: info
  1. Log4j previously allowed to configure which packages to scan for existence of the classes you need to extend if you wish to add a plugin. As you @nodece noted in the link you provided, you don't really need it. You just need the plugin to have the @Plugin annotation, which it does. This mean we can safely remove the packages line from log4j2.yaml.

You do need to verify the Prometheus log4j metrics are still exposed as double check.

@nodece
Copy link
Member Author

nodece commented Mar 29, 2024

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.69%. Comparing base (bbc6224) to head (9e11b7a).
Report is 107 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22327      +/-   ##
============================================
+ Coverage     73.57%   73.69%   +0.11%     
+ Complexity    32624    32390     -234     
============================================
  Files          1877     1885       +8     
  Lines        139502   139867     +365     
  Branches      15299    15320      +21     
============================================
+ Hits         102638   103071     +433     
+ Misses        28908    28832      -76     
- Partials       7956     7964       +8     
Flag Coverage Δ
inttests 27.08% <ø> (+2.50%) ⬆️
systests 24.44% <ø> (+0.12%) ⬆️
unittests 72.98% <ø> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 174 files with indirect coverage changes

@nodece nodece requested a review from lhotari March 30, 2024 10:53
@nodece
Copy link
Member Author

nodece commented Apr 1, 2024

Ping @lhotari

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari merged commit ce4ecd2 into apache:master Apr 1, 2024
48 of 49 checks passed
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Apr 1, 2024
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants