getsentry / sentry-dart

Sentry SDK for Dart and Flutter
https://sentry.io/for/flutter/
MIT License
725 stars 223 forks source link

Migrate Client to HttpClient to offer the proxy setup #392

Open onewilk opened 3 years ago

onewilk commented 3 years ago

In some case, developers need to config httpClient proxy settings to request the public network cause company's network security requirement.

So here is the problem:

sentry_options.dart

set httpClient(Client httpClient) => _httpClient = httpClient ?? _httpClient;

Client is an abstract class from the http lib, and it doesn't offer the proxy setup yet.

Please migrate Client to HttpClient from dart:io.

then developers could set proxy just like this

httpClient.findProxy = (url) => "PROXY xxxx:xxxx";

or just use project's global singleton HttpClient object directly.

Sorry for my poor english ~

marandaneto commented 3 years ago

@onewilk thanks for reporting, we'll discuss this internally and see how much effort :)

marandaneto commented 3 years ago

a downside of it would be that this lib does not support Browser Apps, so we'd need to have a specific Client for Web

bruno-garcia commented 3 years ago

Because of the limitation on dart2js/flutter web, I'm not sure this is the way to go. We could add an abstraction to our use of http, and use our current implementation in the box, but allow users to swap it out as the OP requested.

ueman commented 3 years ago

You can set SentryOptions.client to use a IOClient. The IOClient constructor allows to use a dart:io HttpClient.

That should fix the original issue.

Remarks:

As the issue should be resolved now, I think this can be closed.

We should probably document this somewhere.

What do you think, @marandaneto ?

marandaneto commented 3 years ago

@ueman not really, the idea is that there's an easy way to set up a proxy via options and not replacing the whole transport layer. the current http lib does not allow that, but HttpClient does, but it does not work for Web, so we'd need to think this thoroughly

ueman commented 3 years ago

You can't do a proxy setup in JS though. That's why it isn't available in the HTTP package by default. Using the IOClient from the HTTP package does what the original issue is about. You can do this already and it works for the Dart SDK, Windows and Linux. For platforms with a native integration, I would say this should be done with #265

Example :

import 'dart:io';
import 'package:http/io_client.dart';
import 'package:sentry/sentry.dart';

Sentry.init(
  (options) {
    final httpClient = HttpClient(); // from dart:io
    httpClient.findProxy = (url) => "PROXY xxxx:xxxx";
    final ioClient = IOClient(httpClient); // from package:http
    options.client = ioClient;
  }
)
marandaneto commented 3 years ago

@ueman if that works, sounds good, but instead of people doing such code snippet, we could do something like:

https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/SentryOptions.java#L134-L138

so they configure host, port, user, pass and we do the rest, this should be done then only by non-web env. and also, configure the native platforms for flutter (pass down the configuration when its available via message channel) as they are the ones sending the events.

if the issue is only about the proxy configuration, that's good.

the other question is, do we want to get rid of http package and use only HttpClient which is builtin on the Dart SDK?

ueman commented 3 years ago

the other question is, do we want to get rid of http package and use only HttpClient which is builtin on the Dart SDK?

The built-in client is only available on dart:io. We would have to use XHR (ort he fetch api) on web. So basically we would need to write something like the HTTP package ourself. I would say it's not worth it.

marandaneto commented 3 years ago

sounds good, lets just make it easier to set up the proxy then (internally using the IOClient) as suggested, but let's keep the Client lib, this is not a breaking change anymore then but rather a feat, will remove the 6.0.0 milestone.