-
-
Notifications
You must be signed in to change notification settings - Fork 294
internal(flutter): Add SDK features metadata for SPM vs CocoaPods tracking #3508
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
6617b43
eb80267
67ee1b9
f361bc4
07ddb75
3815350
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,6 +213,12 @@ | |
| infos["integrations"] = integrations.filter { $0 != "SentrySessionReplayIntegration" } | ||
| } | ||
|
|
||
| #if SENTRY_FLUTTER_SPM | ||
| infos["features"] = ["SwiftPackageManager"] | ||
| #else | ||
| infos["features"] = ["Cocoapods"] | ||
|
buenaflor marked this conversation as resolved.
Outdated
|
||
| #endif | ||
|
|
||
| let deviceStr = "device" | ||
| let appStr = "app" | ||
| if let extraContext = PrivateSentrySDKOnly.getExtraContext() as? [String: Any] { | ||
|
|
@@ -241,8 +247,13 @@ | |
|
|
||
| // Not reading the name from PrivateSentrySDKOnly.getSdkName because | ||
| // this is added as a package and packages should follow the sentry-release-registry format | ||
| #if SENTRY_FLUTTER_SPM | ||
| infos["package"] = ["version": PrivateSentrySDKOnly.getSdkVersionString(), | ||
| "sdk_name": "spm:sentry-cocoa"] | ||
| #else | ||
| infos["package"] = ["version": PrivateSentrySDKOnly.getSdkVersionString(), | ||
| "sdk_name": "cocoapods:sentry-cocoa"] | ||
| #endif | ||
|
|
||
| result(infos) | ||
| } | ||
|
|
@@ -276,7 +287,7 @@ | |
| result(debugImages.map { $0.serialize() }) | ||
| } | ||
|
|
||
| private func initNativeSdk(_ call: FlutterMethodCall, result: @escaping FlutterResult) { | ||
|
Check failure on line 290 in packages/flutter/ios/sentry_flutter/Sources/sentry_flutter/SentryFlutterPlugin.swift
|
||
| guard let arguments = call.arguments as? [String: Any], !arguments.isEmpty else { | ||
| print("Arguments is null or empty") | ||
| result(FlutterError(code: "4", message: "Arguments is null or empty", details: nil)) | ||
|
|
@@ -319,6 +330,13 @@ | |
| sdk["integrations"] = integrations | ||
| } | ||
| } | ||
| if let features = flutterSdk!["features"] as? [String] { | ||
| if let sdkFeatures = sdk["features"] as? [String] { | ||
| sdk["features"] = sdkFeatures + features | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if we even need this, but shouldn't we do de-duplication here as well?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can, I just didnt add it because theres almost no chance for a dupe to happen - imo we don't really need it |
||
| } else { | ||
| sdk["features"] = features | ||
| } | ||
|
buenaflor marked this conversation as resolved.
|
||
| } | ||
| event.sdk = sdk | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -260,6 +260,18 @@ class _LoadContextsIntegrationEventProcessor implements EventProcessor { | |
| event.sdk = sdk; | ||
| } | ||
|
|
||
| final featuresList = infos['features'] as List?; | ||
| if (featuresList != null && featuresList.isNotEmpty) { | ||
| final features = List<String>.from(featuresList); | ||
| final sdk = event.sdk ?? _options.sdk; | ||
|
|
||
| for (final feature in features) { | ||
| sdk.addFeature(feature); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here should the merged de-duplication be done, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah when adding/merging to the dart features it already dedupes (addFeature only adds if unique) |
||
| } | ||
|
|
||
| event.sdk = sdk; | ||
| } | ||
|
|
||
| final packageMap = infos['package'] as Map?; | ||
| if (packageMap != null && packageMap.isNotEmpty) { | ||
| final package = Map<String, String>.from(packageMap); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import 'fixture.dart'; | |
| void main() { | ||
| final defaultContexts = { | ||
| 'integrations': ['NativeIntegration'], | ||
| 'features': ['SwiftPackageManager'], | ||
| 'package': {'sdk_name': 'native-package', 'version': '1.0'}, | ||
| 'contexts': { | ||
| 'device': { | ||
|
|
@@ -291,6 +292,39 @@ void main() { | |
| ); | ||
| }); | ||
|
|
||
| group('features', () { | ||
| test('merges features from native into sdk', () async { | ||
| mockLoadContexts(); | ||
| await fixture.registerIntegration(); | ||
|
|
||
| final e = getEvent(); | ||
| final event = | ||
| await fixture.options.eventProcessors.first.apply(e, Hint()); | ||
|
|
||
| expect(event?.sdk?.features.contains('SwiftPackageManager'), true); | ||
| }); | ||
|
|
||
| test('does not duplicate feature if already present', () async { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 馃憤 |
||
| mockLoadContexts({ | ||
| 'features': ['EventFeature'] | ||
| }); | ||
| await fixture.registerIntegration(); | ||
|
|
||
| final sdk = getSdkVersion(); | ||
| sdk.addFeature('EventFeature'); | ||
| final e = getEvent(sdk: sdk); | ||
| final event = | ||
| await fixture.options.eventProcessors.first.apply(e, Hint()); | ||
|
|
||
| expect( | ||
| event?.sdk?.features | ||
| .where((f) => f == 'EventFeature') | ||
| .length, | ||
| 1, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| group('breadcrumbs', () { | ||
| test('takes breadcrumbs from native if scope sync is enabled', () async { | ||
| await fixture.registerIntegration(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.