Fix: Read all environment variables in sentry (#375)

* Correctly read environment variables

* Changelog

* Remove env vars from sentry_flutter

* Docs & Changelog

* Calm Linter

* Hopefully fix failing tests

* Implement PR feedback

* Another try at fixing the failing tests

* Update CHANGELOG.md

Co-authored-by: Manoel Aranda Neto <[email protected]>

* Fix: Set `SentryOptions.debug` in sentry (#376)

* Fix: Set `SentryOptions.debug` in sentry_dart

* Update CHANGELOG.md

Co-authored-by: Manoel Aranda Neto <[email protected]>

Co-authored-by: Manoel Aranda Neto <[email protected]>

* Only override release and dist if not existing

* Apply PR feedback

* add teardown

* Fix path

* PR feedback

Co-authored-by: Manoel Aranda Neto <[email protected]>
Co-authored-by: Manoel Aranda Neto <[email protected]>

Author: ueman

CHANGELOG.md

@@ -7,6 +7,8 @@
 * Bump: sentry-cocoa to v6.2.1 (#360)
 * Feat: Migration from `package_info` to `package_info_plus` plugin (#370)
 * Fix: missing event.origin and event.environment tags for non-native events on iOS (#369)
+* Fix: Set `SentryOptions.debug` in `sentry` (#376)
+* Fix: Read all environment variables in `sentry` (#375)
 
 ## Breaking Changes:
 

dart/lib/src/environment_variables.dart

@@ -0,0 +1,50 @@
+import 'platform_checker.dart';
+
+/// Reads environment variables from the system.
+/// In an Flutter environment these can be set via
+/// `flutter build --dart-define=VARIABLE_NAME=VARIABLE_VALUE`.
+class EnvironmentVariables {
+  static const _sentryEnvironment = 'SENTRY_ENVIRONMENT';
+  static const _sentryDsn = 'SENTRY_DSN';
+  static const _sentryRelease = 'SENTRY_RELEASE';
+  static const _sentryDist = 'SENTRY_DSN';
+
+  /// `SENTRY_ENVIRONMENT`
+  /// See [SentryOptions.environment]
+  String? get environment => const bool.hasEnvironment(_sentryEnvironment)
+      ? const String.fromEnvironment(_sentryEnvironment)
+      : null;
+
+  /// `SENTRY_DSN`
+  /// See [SentryOptions.dsn]
+  String? get dsn => const bool.hasEnvironment(_sentryDsn)
+      ? const String.fromEnvironment(_sentryDsn)
+      : null;
+
+  // `SENTRY_RELEASE`
+  /// See [SentryOptions.release]
+  String? get release => const bool.hasEnvironment(_sentryRelease)
+      ? const String.fromEnvironment(_sentryRelease)
+      : null;
+
+  /// `SENTRY_DIST`
+  /// See [SentryOptions.dist]
+  String? get dist => const bool.hasEnvironment(_sentryDist)
+      ? const String.fromEnvironment(_sentryDist)
+      : null;
+
+  /// Returns an environment based on the compilation mode of Dart or Flutter.
+  /// This can be set as [SentryOptions.environment]
+  String environmentForMode(PlatformChecker checker) {
+    // We infer the enviroment based on the release/non-release and profile
+    // constants.
+
+    if (checker.isReleaseMode()) {
+      return 'production';
+    }
+    if (checker.isProfileMode()) {
+      return 'profile';
+    }
+    return 'debug';
+  }
+}

dart/lib/src/sentry.dart

@@ -1,6 +1,7 @@
 import 'dart:async';
 
 import 'default_integrations.dart';
+import 'environment_variables.dart';
 import 'hub.dart';
 import 'hub_adapter.dart';
 import 'noop_isolate_error_integration.dart'
@@ -52,24 +53,12 @@ class Sentry {
   }
 
   static Future<void> _initDefaultValues(
-      SentryOptions options, AppRunner? appRunner) async {
-    // We infer the enviroment based on the release/non-release and profile
-    // constants.
-    var environment = options.platformChecker.isReleaseMode()
-        ? defaultEnvironment
-        : options.platformChecker.isProfileMode()
-            ? 'profile'
-            : 'debug';
-
-    // if the SENTRY_ENVIRONMENT is set, we read from it.
-    options.environment = const bool.hasEnvironment('SENTRY_ENVIRONMENT')
-        ? const String.fromEnvironment('SENTRY_ENVIRONMENT')
-        : environment;
-
-    // if the SENTRY_DSN is set, we read from it.
-    options.dsn = const bool.hasEnvironment('SENTRY_DSN')
-        ? const String.fromEnvironment('SENTRY_DSN')
-        : options.dsn;
+    SentryOptions options,
+    AppRunner? appRunner,
+  ) async {
+    options.debug = options.platformChecker.isDebugMode();
+
+    _setEnvironmentVariables(options);
 
     // Throws when running on the browser
     if (!isWeb) {
@@ -79,6 +68,25 @@ class Sentry {
     }
   }
 
+  /// This method reads available environment variables and uses them
+  /// accordingly.
+  /// To see which environment variables are available, see [EnvironmentVariables]
+  ///
+  /// The precendence of these options are also described on
+  /// https://docs.sentry.io/platforms/dart/configuration/options/
+  static void _setEnvironmentVariables(SentryOptions options) {
+    final vars = options.environmentVariables;
+    options.dsn = options.dsn ?? vars.dsn;
+
+    if (options.environment == null) {
+      var environment = vars.environmentForMode(options.platformChecker);
+      options.environment = vars.environment ?? environment;
+    }
+
+    options.release = options.release ?? vars.release;
+    options.dist = options.dist ?? vars.dist;
+  }
+
   /// Initializes the SDK
   static Future<void> _init(SentryOptions options, AppRunner? appRunner) async {
     if (isEnabled) {

dart/lib/src/sentry_options.dart

@@ -3,6 +3,7 @@ import 'dart:async';
 import 'package:http/http.dart';
 
 import 'diagnostic_logger.dart';
+import 'environment_variables.dart';
 import 'integration.dart';
 import 'noop_client.dart';
 import 'protocol.dart';
@@ -12,8 +13,9 @@ import 'utils.dart';
 import 'version.dart';
 import 'platform_checker.dart';
 
-/// Default Environment is none is set
-const defaultEnvironment = 'production';
+// TODO: Scope observers, enableScopeSync
+// TODO: shutdownTimeout, flushTimeoutMillis
+// https://api.dart.dev/stable/2.10.2/dart-io/HttpClient/close.html doesn't have a timeout param, we'd need to implement manually
 
 /// Sentry SDK options
 class SentryOptions {
@@ -68,9 +70,6 @@ class SentryOptions {
 
   final List<Integration> _integrations = [];
 
-  // TODO: shutdownTimeout, flushTimeoutMillis
-  // https://api.dart.dev/stable/2.10.2/dart-io/HttpClient/close.html doesn't have a timeout param, we'd need to implement manually
-
   /// Code that provides middlewares, bindings or hooks into certain frameworks or environments,
   /// along with code that inserts those bindings and activates them.
   List<Integration> get integrations => List.unmodifiable(_integrations);
@@ -95,11 +94,13 @@ class SentryOptions {
   BeforeBreadcrumbCallback? beforeBreadcrumb;
 
   /// Sets the release. SDK will try to automatically configure a release out of the box
+  /// See [docs for further information](https://docs.sentry.io/platforms/flutter/configuration/releases/)
   String? release;
 
   /// Sets the environment. This string is freeform and not set by default. A release can be
   /// associated with more than one environment to separate them in the UI Think staging vs prod or
   /// similar.
+  /// See [docs for further information](https://docs.sentry.io/platforms/flutter/configuration/environments/)
   String? environment;
 
   /// Configures the sample rate as a percentage of events to be sent in the range of 0.0 to 1.0. if
@@ -139,7 +140,7 @@ class SentryOptions {
   /// however, when this option is set, stack traces are also sent with messages.
   /// This option, for instance, means that stack traces appear next to all log messages.
   ///
-  /// This option is true` by default.
+  /// This option is `true` by default.
   ///
   /// Grouping in Sentry is different for events with stack traces and without.
   /// As a result, you will get new groups as you enable or disable this flag for certain events.
@@ -149,14 +150,16 @@ class SentryOptions {
   /// This is useful in tests. Should be an implementation of [PlatformChecker].
   PlatformChecker platformChecker = PlatformChecker();
 
+  /// If [environmentVariables] is provided, it is used get the envirnoment
+  /// variables. This is useful in tests.
+  EnvironmentVariables environmentVariables = EnvironmentVariables();
+
   /// When enabled, all the threads are automatically attached to all logged events (Android).
   bool attachThreads = false;
 
   /// Whether to send personal identifiable information along with events
   bool sendDefaultPii = false;
 
-  // TODO: Scope observers, enableScopeSync
-
   SentryOptions({this.dsn}) {
     sdk.addPackage('pub:sentry', sdkVersion);
   }

dart/test/environment_test.dart

@@ -0,0 +1,58 @@
+import 'package:sentry/sentry.dart';
+import 'package:test/test.dart';
+
+import 'mocks.dart';
+import 'mocks/mock_environment_variables.dart';
+
+void main() {
+  // See https://docs.sentry.io/platforms/dart/configuration/options/
+  // and https://github.com/getsentry/sentry-dart/issues/306
+  group('Environment Variables', () {
+    tearDown(() {
+      Sentry.close();
+    });
+
+    test('SentryOptions are not overriden by environment', () async {
+      final options = SentryOptions(dsn: fakeDsn);
+      options.release = 'release-1.2.3';
+      options.dist = 'foo';
+      options.environment = 'prod';
+      options.environmentVariables = MockEnvironmentVariables(
+        dsn: 'foo-bar',
+        environment: 'staging',
+        release: 'release-9.8.7',
+        dist: 'bar',
+      );
+
+      await Sentry.init(
+        (options) => options,
+        options: options,
+      );
+
+      expect(options.dsn, fakeDsn);
+      expect(options.environment, 'prod');
+      expect(options.release, 'release-1.2.3');
+      expect(options.dist, 'foo');
+    });
+
+    test('SentryOptions are overriden by environment', () async {
+      final options = SentryOptions();
+      options.environmentVariables = MockEnvironmentVariables(
+        dsn: 'foo-bar',
+        environment: 'staging',
+        release: 'release-9.8.7',
+        dist: 'bar',
+      );
+
+      await Sentry.init(
+        (options) => options,
+        options: options,
+      );
+
+      expect(options.dsn, 'foo-bar');
+      expect(options.environment, 'staging');
+      expect(options.release, 'release-9.8.7');
+      expect(options.dist, 'bar');
+    });
+  });
+}

dart/test/fake_platform_checker.dart

@@ -1,7 +1,7 @@
 import 'package:sentry/src/platform_checker.dart';
 
 /// Fake class to be used in unit tests for mocking different environments
-class FakePlatformChecker implements PlatformChecker {
+class FakePlatformChecker extends PlatformChecker {
   FakePlatformChecker.releaseMode() {
     _releaseMode = true;
     _debugMode = false;

dart/test/mocks/mock_environment_variables.dart

@@ -0,0 +1,30 @@
+import 'package:sentry/src/environment_variables.dart';
+
+class MockEnvironmentVariables extends EnvironmentVariables {
+  MockEnvironmentVariables({
+    String? dist,
+    String? dsn,
+    String? environment,
+    String? release,
+  })  : _dist = dist,
+        _dsn = dsn,
+        _environment = environment,
+        _release = release;
+
+  final String? _dist;
+  final String? _dsn;
+  final String? _environment;
+  final String? _release;
+
+  @override
+  String? get dist => _dist;
+
+  @override
+  String? get dsn => _dsn;
+
+  @override
+  String? get environment => _environment;
+
+  @override
+  String? get release => _release;
+}

dart/test/sentry_test.dart

@@ -185,6 +185,7 @@ void main() {
     await Sentry.init((options) {
       options.dsn = fakeDsn;
       expect(options.environment, 'debug');
+      expect(options.debug, true);
     }, options: sentryOptions);
   });
 
@@ -194,6 +195,7 @@ void main() {
     await Sentry.init((options) {
       options.dsn = fakeDsn;
       expect(options.environment, 'profile');
+      expect(options.debug, false);
     }, options: sentryOptions);
   });
 
@@ -202,7 +204,8 @@ void main() {
       ..platformChecker = FakePlatformChecker.releaseMode();
     await Sentry.init((options) {
       options.dsn = fakeDsn;
-      expect(options.environment, defaultEnvironment);
+      expect(options.environment, 'production');
+      expect(options.debug, false);
     }, options: sentryOptions);
   });
 }

flutter/lib/src/default_integrations.dart

@@ -340,8 +340,7 @@ class LoadAndroidImageListIntegration
 /// a PackageInfo wrapper to make it testable
 typedef PackageLoader = Future<PackageInfo> Function();
 
-/// an Integration that loads the Release version from Native Apps
-/// or SENTRY_RELEASE and SENTRY_DIST variables
+/// An [Integration] that loads the release version from native apps
 class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
   final PackageLoader _packageLoader;
 
@@ -351,22 +350,15 @@ class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
   FutureOr<void> call(Hub hub, SentryFlutterOptions options) async {
     try {
       if (!kIsWeb) {
-        final packageInfo = await _packageLoader();
-        final release =
-            '${packageInfo.packageName}@${packageInfo.version}+${packageInfo.buildNumber}';
-        options.logger(SentryLevel.debug, 'release: $release');
-
-        options.release = release;
-        options.dist = packageInfo.buildNumber;
-      } else {
-        // for non-mobile builds, we read the release and dist from the
-        // system variables (SENTRY_RELEASE and SENTRY_DIST).
-        options.release = const bool.hasEnvironment('SENTRY_RELEASE')
-            ? const String.fromEnvironment('SENTRY_RELEASE')
-            : options.release;
-        options.dist = const bool.hasEnvironment('SENTRY_DIST')
-            ? const String.fromEnvironment('SENTRY_DIST')
-            : options.dist;
+        if (options.release == null || options.dist == null) {
+          final packageInfo = await _packageLoader();
+          final release =
+              '${packageInfo.packageName}@${packageInfo.version}+${packageInfo.buildNumber}';
+          options.logger(SentryLevel.debug, 'release: $release');
+
+          options.release = options.release ?? release;
+          options.dist = options.dist ?? packageInfo.buildNumber;
+        }
       }
     } catch (error) {
       options.logger(

flutter/lib/src/sentry_flutter.dart

@@ -60,8 +60,6 @@ mixin SentryFlutter {
     SentryFlutterOptions options,
     MethodChannel channel,
   ) async {
-    options.debug = kDebugMode;
-
     // web still uses a http transport for Web which is set by default
     if (!kIsWeb) {
       options.transport = FileSystemTransport(channel, options);