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

add sentry-okhttp module #3005

Merged
merged 7 commits into from
Nov 24, 2023
Merged

Conversation

ToppleTheNun
Copy link
Contributor

📜 Description

Add sentry-okhttp module, update existing classes in sentry-android-okhttp to use it, and mark existing classes in sentry-android-okhttp as deprecated.

💡 Motivation and Context

I use OkHttp in a purely Java project and currently maintain a fork of sentry-okhttp-android to enable us to use it, which causes us to miss upstream changes from time to time. This would enable us, and others, to be able to use an official Sentry SDK for instrumenting OkHttp.

Closes #1783.

💚 How did you test it?

I use a very similar version to the one in this PR in a project and it works. I also copied the existing tests from sentry-android-okhttp over and made sure they passed without changes.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@adinauer
Copy link
Member

adinauer commented Nov 3, 2023

Thanks for the PR @ToppleTheNun sorry for the delay here. I quickly tested and looks good on a Spring backend application. Still have to review the PR, but this also needs a review from one of @romtsn @markushi or @stefanosiano so we make sure not to break existing users of sentry-android-okhttp.

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Left some comments. Leaving the serious part of the review to Android folks.

.craft.yml Outdated Show resolved Hide resolved
sentry-android-okhttp/build.gradle.kts Show resolved Hide resolved
@ToppleTheNun
Copy link
Contributor Author

I'm also happy to point this at the 7.0.0 branch since this could be considered a breaking change.

@romtsn
Copy link
Member

romtsn commented Nov 7, 2023

I'm also happy to point this at the 7.0.0 branch since this could be considered a breaking change.

Yeah, I think it's a good idea to make it part of 7.0.0. I will get to this PR a bit later and change whatever is needed, but your initial work is really appreciated, thank you a lot 🙏 We'll make it part of the next 7.0.0-rc.2

@ToppleTheNun ToppleTheNun changed the base branch from main to feat/7.0.0 November 7, 2023 21:36
@ToppleTheNun ToppleTheNun changed the base branch from feat/7.0.0 to main November 7, 2023 21:36
@romtsn romtsn changed the base branch from main to feat/7.0.0 November 17, 2023 09:30
@ToppleTheNun
Copy link
Contributor Author

I updated it to be based correctly on the 7.0.0 branch.

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

Thanks again @ToppleTheNun, great stuff! I've updated the PR with a couple of things:

  • Use delegation instead of inheritance, so we don't change the class signature and break binary compatibility
  • Update changelog to reflect exactly what changed
  • Remove SentryOkHttpUtils from public api as it was exposed by mistake most likely

@romtsn romtsn merged commit 209ee17 into getsentry:feat/7.0.0 Nov 24, 2023
15 of 16 checks passed
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.

Package OkHttp as a jar instead of aar
3 participants