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

Use 'HttpMessageHandler' instead of 'HttpClientHandler' to support NSUrlSessionHandler on iOS #2503

Merged
merged 12 commits into from Jul 25, 2023

Conversation

AlexSchuetz
Copy link
Contributor

fixing #2502

Breaking change:

  • SentryOptions.HttpClientHandlerFactory moved to SentryOptions.HttpMessageHandlerFactory

AlexSchuetz and others added 2 commits July 21, 2023 11:58
@bitsandfoxes
Copy link
Contributor

That's a great catch, thanks for contributing!
We'd like to avoid introducing breaking changes outside of mayor releases and the next one is still a bit out.
I'm thinking, could this be added as an additional option and we mark the old one as obsolete?

@AlexSchuetz
Copy link
Contributor Author

Of course. That's a good idea. I will adjust it shortly. 👍

@jamescrosswell
Copy link
Collaborator

Thanks heaps @AlexSchuetz, the commit all looks great!

There are a couple of things that need doing to get things through our CI servers though.

Changelog

Could you add the following to the CHANGELOG.md in the Fixes section?

- Fixes #2502 SentryOptions should use 'HttpMessageHandler' instead of 'HttpClientHandler' to support NSUrlSessionHandler on iOS  ([#2503](https://github.com/getsentry/sentry-dotnet/pull/2503))

Update verify expectations

We're using Verify to make sure that any changes to the API surface area are deliberate. That means when the API does change, we need to update the relevant verified.txt files. The easiest way to create updated versions of those is to run ./build.sh or build.cmd (depending on your development platform) from the repository root folder. So if you're developing on MacOS, for example, you'd run ./build.sh. Once you've done that, I think you should see changes to commit for the following files:

  • test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt
  • test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt
  • test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt
  • test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt

You can add those to a new commit with a description like "updated verify files" or something like that and push.

Note: You can ignore any errors the build shows relating to profiling... that's a long story and unrelated to this PR.

Failed Checks

There are a couple of other checks that are failing. I'm not actually sure what's going on there. @bitsandfoxes do you have any context for that?

@AlexSchuetz
Copy link
Contributor Author

I did the required commit and also had a look at the error. NSUrlSessionHandler does not support a proxy, but I want to include the code for setting it anyway, because if it ever does noone will remember adjusting it here.
Thus I implemented it and placed it inside of a try-catch. So nothing should go wrong. I now adjusted my implementation and check NSUrlSessionHandler.SupportsProxy. Hopefully this works.

@AlexSchuetz
Copy link
Contributor Author

I investigated a bit more. It is a code analysis error. Since NSUrlSessionHandler.Proxy is not supported.
I added a pragma for this one line to disable this issue, since I want to support it as soon as NSUrlSesseionHandler does.

If you think I should remove the code in question and just print out that a proxy for NSUrlSessionHandler is not supported, that would be fine as well. I don't know if the Proxy-property will ever be implemented. I think iOS and MAC require a proxy to be defined elsewhere.

@bruno-garcia bruno-garcia changed the title fix #2502 Use 'HttpMessageHandler' instead of 'HttpClientHandler' to support NSUrlSessionHandler on iOS Jul 24, 2023
Copy link
Member

@bruno-garcia bruno-garcia 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 minor suggestions but generally lgtm

src/Sentry/Internal/Http/DefaultSentryHttpClientFactory.cs Outdated Show resolved Hide resolved
src/Sentry/SentryOptions.cs Outdated Show resolved Hide resolved
src/Sentry/Internal/Http/DefaultSentryHttpClientFactory.cs Outdated Show resolved Hide resolved
@AlexSchuetz
Copy link
Contributor Author

@bruno-garcia I followed your suggestions. Am I supposed to resolve those conversations now?

@bitsandfoxes
Copy link
Contributor

Am I supposed to resolve those conversations now?

Yes. Everything that has been addressed can be marked as resolved.

@bitsandfoxes bitsandfoxes self-requested a review July 25, 2023 13:03
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

This looks good to me! Thanks again for contributing!

@bitsandfoxes bitsandfoxes merged commit 1608a84 into getsentry:main Jul 25, 2023
7 checks passed
@AlexSchuetz
Copy link
Contributor Author

@bitsandfoxes @bruno-garcia You are welcome. Thank's for your support and review.

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

4 participants