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

Allow dynamically setting the log level even for loggers that aren't already explicitly configured #13156

Merged
merged 2 commits into from
May 16, 2024

Conversation

yashmayya
Copy link
Collaborator

@yashmayya yashmayya commented May 15, 2024

  • This is a fix for Dynamic log configuration APIs don't allow setting the log level for loggers that aren't explicitly configured #13155
  • Support for dynamic log configuration APIs was added in Adding logger utils and allow change logger level at runtime #9180 and documented in add documentation for dynamic logging pinot-contrib/pinot-docs#132
  • Currently, these APIs don't allow modifying the level of loggers that aren't explicitly configured.
  • For instance, let's say that a Pinot controller process is started with the log4j configuration file containing log levels defined for the following loggers: root, org.apache.pinot.tools.admin and org.reflections (see this log4j config file for reference).
  • Now, if we attempt to change the log level of the org.apache.pinot.controller.helix.core.realtime.SegmentCompletionManager class via PUT /loggers/org.apache.pinot.controller.helix.core.realtime.SegmentCompletionManager?level=DEBUG, this returns the following response: {"code":500,"error":"Logger - org.apache.pinot.controller.helix.core.realtime.SegmentCompletionManager not found"}
  • This is because the check here only checks whether the logger name is in the list of loggers that are already explicitly configured.
  • This severely limits the usefulness of the dynamic log configuration API as it prevents the modification of all loggers that aren't explicitly configured in the log4j configuration file.
  • This patch allows dynamically setting the log level even for loggers that aren't already explicitly configured. In a future enhancement, we could also allow dynamically setting the log level for an ancestor logger which would affect all the descendant loggers (see https://logging.apache.org/log4j/2.x/manual/architecture.html#logger-hierarchy).
  • This also allows dynamically configuring the log level for ancestor / parent loggers - for instance, org.apache.pinot.controller.helix.core. This will then also affect the log level for all the descendant / child loggers - for instance, org.apache.pinot.controller.helix.core.realtime.SegmentCompletionManager (see https://logging.apache.org/log4j/2.x/manual/architecture.html#logger-hierarchy).
  • Suggested label: bugfix or feature

@codecov-commenter
Copy link

codecov-commenter commented May 15, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 62.15%. Comparing base (59551e4) to head (c3356b3).
Report is 444 commits behind head on master.

Files Patch % Lines
.../pinot/broker/api/resources/PinotBrokerLogger.java 0.00% 1 Missing ⚠️
...ontroller/api/resources/PinotControllerLogger.java 0.00% 1 Missing ⚠️
.../pinot/minion/api/resources/PinotMinionLogger.java 0.00% 1 Missing ⚠️
.../pinot/server/api/resources/PinotServerLogger.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #13156      +/-   ##
============================================
+ Coverage     61.75%   62.15%   +0.40%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2515      +79     
  Lines        133233   137871    +4638     
  Branches      20636    21337     +701     
============================================
+ Hits          82274    85692    +3418     
- Misses        44911    45779     +868     
- Partials       6048     6400     +352     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration <0.01% <0.00%> (-0.01%) ⬇️
integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration2 0.00% <0.00%> (ø)
java-11 62.12% <81.81%> (+0.41%) ⬆️
java-21 27.81% <0.00%> (-33.82%) ⬇️
skip-bytebuffers-false 62.14% <81.81%> (+0.39%) ⬆️
skip-bytebuffers-true 27.78% <0.00%> (+0.06%) ⬆️
temurin 62.15% <81.81%> (+0.40%) ⬆️
unittests 62.14% <81.81%> (+0.40%) ⬆️
unittests1 46.69% <100.00%> (-0.20%) ⬇️
unittests2 27.81% <0.00%> (+0.08%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xiangfu0 xiangfu0 merged commit 8bb81b8 into apache:master May 16, 2024
20 checks passed
gortiz pushed a commit to gortiz/pinot that referenced this pull request Jun 14, 2024
…already explicitly configured (apache#13156)

* Allow dynamically setting the log level even for loggers that aren't already explicitly configured

* Allow dynamically configuring log levels for ancestor / parent loggers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants