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

test: Roll SLF4J log4j bindings to v2 #1190

Merged
merged 10 commits into from
Jun 21, 2023
Merged

test: Roll SLF4J log4j bindings to v2 #1190

merged 10 commits into from
Jun 21, 2023

Conversation

scottgerring
Copy link
Contributor

@scottgerring scottgerring commented Jun 13, 2023

Issue #, if available: #1180

Description of changes:

We generally couple directly against Log4j, but in a couple of places we write to the slf4j API, which is then adapted back to log4j by log4j's log4j-slf4j-impl package:

https://github.com/awslabs/aws-lambda-powertools-java/blob/76ca20f2df7631b6cf4eaed852a55ab95f27be1d/powertools-serialization/pom.xml#L52-L55

Because this adapter is being pulled in by our code and is using slf4j-v1, customers using slf4j-v2 themselves can have issues using the powertools (see #1180). This PR switches the adapter to slf4j-v2.

Long term we intend to switch to slf4j (#995, #965), rather than coupling directly against log4j. As this will be a breaking change, it has been earmarked for powertools for java v2.

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@scottgerring
Copy link
Contributor Author

@jeromevdl what do you think? This seems pretty harmless to me, and the slf4j upgrade-to-v2 FAQ lists it as a non-breaking change.

I have also tried to document explicitly on this PR the state of logging in the repo and our long term plans here.

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (7f804fe) 70.87% compared to head (5d0570c) 70.87%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1190   +/-   ##
=========================================
  Coverage     70.87%   70.87%           
  Complexity      541      541           
=========================================
  Files            72       72           
  Lines          2328     2328           
  Branches        254      254           
=========================================
  Hits           1650     1650           
  Misses          558      558           
  Partials        120      120           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

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

Looks ok.

That's all the slf4j reference we have ? I still don't understand why we introduced slf4j here...

@msailes any reason for this; Was it to fix a warning in the log: #123 ?

@scottgerring
Copy link
Contributor Author

Looks ok.

That's all the slf4j reference we have ? I still don't understand why we introduced slf4j here...

@msailes any reason for this; Was it to fix a warning in the log: #123 ?

You can see that some of our code couples against it instead of log4j. I don't know if it was initially an autocomplete accident or good intentions ("we should be using slf4j not log4j").

@jeromevdl
Copy link
Contributor

@scottgerring anything else to do on this one? it is marked as draft...

@scottgerring scottgerring marked this pull request as ready for review June 21, 2023 06:48
scottgerring and others added 4 commits June 21, 2023 14:48
Bumps [constructs](https://github.com/aws/constructs) from 10.2.47 to 10.2.55.
- [Release notes](https://github.com/aws/constructs/releases)
- [Commits](aws/constructs@v10.2.47...v10.2.55)

---
updated-dependencies:
- dependency-name: software.constructs:constructs
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@scottgerring scottgerring requested a review from jeromevdl June 21, 2023 12:20
@jeromevdl jeromevdl merged commit 31bebb7 into main Jun 21, 2023
@jeromevdl jeromevdl deleted the slf4j1-to-2 branch June 21, 2023 12:38
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