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

Support Spring 6 and Spring Boot 3 #2289

Merged
merged 29 commits into from Oct 18, 2022
Merged

Support Spring 6 and Spring Boot 3 #2289

merged 29 commits into from Oct 18, 2022

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Oct 11, 2022

📜 Description

Support jakarta which is required for Spring 6 and Spring Boot 3.
Also need to bump environment to Java 17 for building.

💡 Motivation and Context

Newer versions of Spring (Boot) use jakarta imports instead of javax imports that were used in older versions.

💚 How did you test it?

Samples

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

  • Some tests have been removed (@Ignore), they should be reactivated
  • Test coverage min has been set to 0 (jacoco), it should be set back to 0.6
  • Some explicit dependencies have been added to force newer versions (byte-buddy, asm), maybe there's a way to avoid that

@adinauer adinauer changed the base branch from main to feat/6.6.x October 13, 2022 09:57
@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 317.31 ms 353.55 ms 36.24 ms
Size 1.73 MiB 2.29 MiB 580.38 KiB

Previous results on branch: gh-1984

Startup times

Revision Plain With Sentry Diff
b0b8572 350.30 ms 354.92 ms 4.62 ms
7902403 343.80 ms 389.35 ms 45.55 ms
6629cd8 318.63 ms 350.76 ms 32.13 ms
b9eab61 308.98 ms 339.26 ms 30.28 ms
84c97c5 284.34 ms 317.28 ms 32.94 ms

App size

Revision Plain With Sentry Diff
b0b8572 1.73 MiB 2.29 MiB 579.57 KiB
7902403 1.73 MiB 2.29 MiB 579.57 KiB
6629cd8 1.73 MiB 2.29 MiB 579.57 KiB
b9eab61 1.73 MiB 2.29 MiB 579.88 KiB
84c97c5 1.73 MiB 2.29 MiB 579.88 KiB

@adinauer adinauer marked this pull request as ready for review October 13, 2022 14:25
@@ -0,0 +1,19 @@
# Sentry Sample Spring Boot

Sample application showing how to use Sentry with [Spring boot](http://spring.io/projects/spring-boot).
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should mention that it's Spring boot Jakarta or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Need to check how exactly the duplication stuff works here. Can add if easily possible

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah it's on the samples.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@@ -0,0 +1,19 @@
# Sentry Sample Spring

Sample application showing how to use Sentry with [Spring](http://spring.io/).
Copy link
Member

Choose a reason for hiding this comment

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

also Jakarta is missing here

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above. Will check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

}
}

task("jakartaTransformation", JavaExec::class) {
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this transform in the samples actually? Can't we just manually have samples with the correct package name?

Copy link
Member

Choose a reason for hiding this comment

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

also, I don't see that this task depends on anything, so we'd need to manually run it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it was a one time command to initially clone the samples. Not sure if we need it again in case we change a lot of stuff. @lbloder do you think we should keep this in case we need to clone again?

@romtsn
Copy link
Member

romtsn commented Oct 14, 2022

I think I need a better understanding of how this supposed to work to properly review it. What's the supposed workflow for this? If we change something in the sentry-spring-boot-starter, we should then go and manually trigger a build in the sentry-spring-boot-starter-jakarta module? How does this work with the resources folder, I assume the transform works only source code right?

Do we really need to have all those classes under source control, since they are synthetic? Can't we just have symlinks or something and generate them on the fly (e.g. only when running CI checks + when publishing)?

@adinauer
Copy link
Member Author

If we don't have it checked in, how do we add the dependency on it for the sample? We'd need to publish it somewhere, no?

@adinauer
Copy link
Member Author

Started an internal discussion whether it makes sense to move forward with this approach based on Eclipse Transformer. It feels like a lot of complexity just to save us from maintaining a few more files. That said, I'd like this to be released as an initial alpha asap so people can get their hands on it and provide feedback.

@@ -27,7 +27,7 @@ jobs:
uses: actions/setup-java@v2
with:
java-version: ${{ matrix.java }}
distribution: 'adopt'
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to change the distribution?

Choose a reason for hiding this comment

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

I think this is a good move since they have rebranded and recommend people move from Adopt to Termurin.

See - https://blog.adoptopenjdk.net/2021/08/goodbye-adoptopenjdk-hello-adoptium/

@@ -1,5 +1,5 @@
# Daemon’s heap size
org.gradle.jvmargs=-Xmx4g -XX:MaxPermSize=512m -XX:MaxMetaspaceSize=1536m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8 -XX:+UseParallelGC
org.gradle.jvmargs=-Xmx4g -XX:MaxMetaspaceSize=1536m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8 -XX:+UseParallelGC
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, MaxPermSize was helping with Android builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

build.gradle.kts Outdated Show resolved Hide resolved
@marandaneto
Copy link
Contributor

Is there a specific commit hash we should look into? 300 files are hard to review, and GH does not render correctly.
I'd recommend all refactoring such as org.mockito.kotlin going to separate PRs.

@adinauer
Copy link
Member Author

Is there a specific commit hash we should look into?

No. The most important changes are probably the build files for the new modules. I guess it's easier to check out the branch and view them in the IDE. After this has been released my preference would be to rework them, remove the automated transformation code and just have two sets of gradle modules for each of the spring modules.

@adinauer adinauer merged commit 0b78c36 into feat/6.7.x Oct 18, 2022
@adinauer adinauer deleted the gh-1984 branch October 18, 2022 07:05
@adinauer adinauer changed the title Support newer versions of Spring and Spring Boot Support Spring 6 and Spring Boot 3 Oct 18, 2022
@github-actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Support Spring 6 and Spring Boot 3 ([#2289](https://github.com/getsentry/sentry-java/pull/2289))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against b253c97

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants