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

Adding logger utils and allow change logger level at runtime #9180

Merged
merged 1 commit into from Aug 9, 2022

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Aug 7, 2022

Allow change logger level at runtime.
APIs:

  1. Fetch all the loggers:
GET /loggers
  1. Fetch logger information:
GET /loggers/${loggerName}
  1. Update logger level:
PUT /loggers/${loggerName}?level=${logLevel}

Sample usage:

➜  ~ curl -X GET -H "accept: application/json" localhost:8000/loggers
["root","org.reflections","org.apache.pinot.tools.admin"]
➜  ~ curl -X GET -H "accept: application/json" localhost:8000/loggers/root
{"filter":null,"level":"INFO","name":"root"}
➜  ~ curl -X GET -H "accept: application/json" localhost:8000/loggers
["root","org.reflections","org.apache.pinot.tools.admin"]
➜  ~ curl -X GET -H "accept: application/json" localhost:8000/loggers/root
{"filter":null,"level":"INFO","name":"root"}
➜  ~ curl -X PUT -H "accept: application/json" localhost:8000/loggers/root?level=DEBUG
{"filter":null,"level":"DEBUG","name":"root"}
➜  ~ curl -X GET -H "accept: application/json" localhost:8000/loggers/root
{"filter":null,"level":"DEBUG","name":"root"}
➜  ~ curl -X PUT -H "accept: application/json" localhost:8000/loggers/root?level=ERROR
{"filter":null,"level":"ERROR","name":"root"}
➜  ~ curl -X GET -H "accept: application/json" localhost:8000/loggers/root
{"filter":null,"level":"ERROR","name":"root"}

@xiangfu0 xiangfu0 added the release-notes Referenced by PRs that need attention when compiling the next release notes label Aug 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2022

Codecov Report

Merging #9180 (5395c67) into master (449f89a) will increase coverage by 41.48%.
The diff coverage is 45.45%.

@@              Coverage Diff              @@
##             master    #9180       +/-   ##
=============================================
+ Coverage     28.49%   69.98%   +41.48%     
- Complexity       53     4756     +4703     
=============================================
  Files          1836     1852       +16     
  Lines         98103    98760      +657     
  Branches      14897    15021      +124     
=============================================
+ Hits          27956    69113    +41157     
+ Misses        67460    24773    -42687     
- Partials       2687     4874     +2187     
Flag Coverage Δ
integration1 26.20% <0.00%> (-0.16%) ⬇️
integration2 24.82% <0.00%> (-0.02%) ⬇️
unittests1 67.14% <92.59%> (?)
unittests2 15.26% <0.00%> (?)

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

Impacted Files Coverage Δ
.../pinot/broker/api/resources/PinotBrokerLogger.java 0.00% <0.00%> (ø)
...ontroller/api/resources/PinotControllerLogger.java 0.00% <0.00%> (ø)
.../pinot/minion/api/resources/PinotMinionLogger.java 0.00% <0.00%> (ø)
.../pinot/server/api/resources/PinotServerLogger.java 0.00% <0.00%> (ø)
...ava/org/apache/pinot/common/utils/LoggerUtils.java 92.59% <92.59%> (ø)
...ntroller/helix/core/minion/TaskMetricsEmitter.java 34.88% <0.00%> (-51.17%) ⬇️
...re/query/reduce/SelectionOnlyStreamingReducer.java 52.94% <0.00%> (-28.02%) ⬇️
...n/java/org/apache/pinot/common/utils/URIUtils.java 66.66% <0.00%> (-7.41%) ⬇️
...core/operator/filter/predicate/PredicateUtils.java 68.18% <0.00%> (-5.90%) ⬇️
...not/broker/broker/helix/ClusterChangeMediator.java 75.26% <0.00%> (-5.38%) ⬇️
... and 1293 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@xiangfu0 xiangfu0 force-pushed the adding_logger_resource branch 3 times, most recently from 201b3b9 to c6e0180 Compare August 7, 2022 23:03
@xiangfu0 xiangfu0 changed the title Adding ability to update logger level Adding logger utils and allow change logger level at runtime Aug 7, 2022
@xiangfu0 xiangfu0 force-pushed the adding_logger_resource branch 4 times, most recently from c7b817b to ba45cf2 Compare August 7, 2022 23:44
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Can you please add some tests for the LoggerUtils?
We want to test the following things:

  1. The expected return for each method
  2. Whether the already initialized logger can be updated

@xiangfu0 xiangfu0 force-pushed the adding_logger_resource branch 2 times, most recently from c3094a5 to 70dd3c6 Compare August 8, 2022 21:37
@xiangfu0 xiangfu0 force-pushed the adding_logger_resource branch 3 times, most recently from b9e98fb to 7228fa2 Compare August 9, 2022 00:37
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@xiangfu0 xiangfu0 merged commit 6ed17d2 into apache:master Aug 9, 2022
@xiangfu0 xiangfu0 deleted the adding_logger_resource branch August 9, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Referenced by PRs that need attention when compiling the next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants