{{ message }}
add sentry-okhttp module - #3005
Merged
Merged
Conversation
ToppleTheNun
requested review from
adinauer,
markushi,
romtsn and
stefanosiano
as code owners
October 20, 2023 22:38
Member
adinauer
reviewed
Nov 6, 2023
adinauer
left a comment
adinauer
left a comment
Member
There was a problem hiding this comment.
Left some comments. Leaving the serious part of the review to Android folks.
Contributor
Author
|
I'm also happy to point this at the 7.0.0 branch since this could be considered a breaking change. |
ToppleTheNun
force-pushed
the
sentry-okhttp
branch
from
November 6, 2023 21:38
3461cc1 to
9105898
Compare
Member
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 |
ToppleTheNun
force-pushed
the
sentry-okhttp
branch
from
November 17, 2023 18:55
9105898 to
67a21f4
Compare
partially resolves getsentry#1783
ToppleTheNun
force-pushed
the
sentry-okhttp
branch
from
November 17, 2023 19:37
67a21f4 to
1a52a5f
Compare
Contributor
Author
romtsn
approved these changes
Nov 24, 2023
romtsn
left a comment
romtsn
left a comment
Member
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Description
Add
sentry-okhttpmodule, update existing classes insentry-android-okhttpto use it, and mark existing classes insentry-android-okhttpas deprecated.💡 Motivation and Context
I use OkHttp in a purely Java project and currently maintain a fork of
sentry-okhttp-androidto 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-okhttpover and made sure they passed without changes.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps