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.

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

Fix multiple logger binding error #3498

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Fix multiple logger binding error #3498

merged 1 commit into from
Dec 7, 2023

Conversation

Cali0707
Copy link
Member

Fixes #3452

Proposed Changes

  • Instead of swapping out the "real" logger for a nop logger, just use the real logger in benchmarks. Bonus: this is a better measure of the real performance!

Testing locally, the benchmarks work now. To verify for yourself, run ./hack/run.sh benchmark-filters and wait until the first benchmark runs, then verify that you don't get a message along the lines of:

SLF4J: Class path contains multiple SLF4J providers.
SLF4J: Found provider [ch.qos.logback.classic.spi.LogbackServiceProvider@66202ab4]
SLF4J: Found provider [org.slf4j.nop.NOPServiceProvider@5f731a2f]
SLF4J: See https://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual provider is of type [ch.qos.logback.classic.spi.LogbackServiceProvider@66202ab4]

@Cali0707
Fix multiple logger binding error
11a084c 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow Knative Prow
Copy link

knative-prow bot commented Nov 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/data-plane size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 30, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 30, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9a4a161) 61.52% compared to head (11a084c) 61.52%.
Report is 11 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #3498   +/-   ##
=========================================
  Coverage     61.52%   61.52%           
  Complexity      764      764           
=========================================
  Files           181      181           
  Lines         12414    12414           
  Branches        266      266           
=========================================
  Hits           7638     7638           
  Misses         4165     4165           
  Partials        611      611
Flag Coverage Δ
java-unittests 70.83% <ø> (ø)

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.

@Leo6Leo
Copy link
Contributor

Leo6Leo commented Dec 1, 2023

/lgtm
/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2023
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2023
@Cali0707
Copy link
Member Author

Cali0707 commented Dec 1, 2023

Do you think we can unhold this PR @pierDipi @creydr ?

@pierDipi
Copy link
Member

pierDipi commented Dec 5, 2023

/unhold
/retest

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2023
@Cali0707
Copy link
Member Author

Cali0707 commented Dec 5, 2023

/retest-required

@Cali0707
Copy link
Member Author

Cali0707 commented Dec 5, 2023

/retest

@Cali0707
Copy link
Member Author

Cali0707 commented Dec 5, 2023

/retest-required

3 similar comments
@Cali0707
Copy link
Member Author

Cali0707 commented Dec 5, 2023

/retest-required

@Cali0707
Copy link
Member Author

Cali0707 commented Dec 5, 2023

/retest-required

@Cali0707
Copy link
Member Author

Cali0707 commented Dec 5, 2023

/retest-required

@Cali0707
Copy link
Member Author

Cali0707 commented Dec 6, 2023

/retest

1 similar comment
@matzew
Copy link
Contributor

matzew commented Dec 7, 2023

/retest

@knative-prow knative-prow bot merged commit 9fa7d77 into knative-extensions:main Dec 7, 2023
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Leo6Leo Leo6Leo Awaiting requested review from Leo6Leo

@creydr creydr Awaiting requested review from creydr

@pierDipi pierDipi Awaiting requested review from pierDipi

Assignees

@Leo6Leo Leo6Leo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/data-plane lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix Benchmarks logger multiple bindings problem/performance regression
4 participants
@Cali0707 @Leo6Leo @pierDipi @matzew