Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
xiangfu0 merged 1 commit into from
Aug 9, 2022
Merged

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

xiangfu0 merged 1 commit into from
Aug 9, 2022

Conversation

@xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Aug 7, 2022 •
edited
Loading

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 •
edited
Loading

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
@xiangfu0 xiangfu0 mentioned this pull request Aug 8, 2022
Closed
Jackie-Jiang
Jackie-Jiang reviewed Aug 8, 2022
View reviewed changes
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
Jackie-Jiang
Jackie-Jiang reviewed Aug 9, 2022
View reviewed changes
@xiangfu0 xiangfu0 force-pushed the adding_logger_resource branch 3 times, most recently from b9e98fb to 7228fa2 Compare August 9, 2022 00:37
@xiangfu0 xiangfu0 requested a review from Jackie-Jiang August 9, 2022 00:39
Jackie-Jiang
Jackie-Jiang approved these changes Aug 9, 2022
View reviewed changes
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 force-pushed the adding_logger_resource branch from 7228fa2 to c5bdf14 Compare August 9, 2022 18:56
@xiangfu0 xiangfu0 force-pushed the adding_logger_resource branch from c5bdf14 to 5395c67 Compare August 9, 2022 19:00
@xiangfu0 xiangfu0 merged commit 6ed17d2 into apache:master Aug 9, 2022
@xiangfu0 xiangfu0 deleted the adding_logger_resource branch August 9, 2022 20:28
@agavra agavra mentioned this pull request Oct 19, 2022
Merged
This was referenced May 15, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jackie-Jiang Jackie-Jiang Jackie-Jiang approved these changes

@kishoreg kishoreg Awaiting requested review from kishoreg

@walterddr walterddr Awaiting requested review from walterddr

+1 more reviewer

@kkrugler kkrugler kkrugler left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@xiangfu0 @codecov-commenter @kkrugler @walterddr @Jackie-Jiang