Adds EmbraceConfigurable. Allows Option to be passed in when app_id not present to override EmbraceConfig behavior #56

atreat · 2024-09-06T19:41:55Z

The existing EmbraceConfig object becomes a pass through to the underlying implementation/delegate. This is an object that conforms to the EmbraceConfigurable protocol. This protocol declares the properties that the SDK uses during its runtime. It also includes an update method that is used if/when the implementation should refresh its values (if necessary).

This PR includes a static implementation of EmbraceConfigurable called DefaultConfig, which is the default object when initializing Embrace.Options without an Embrace appId.

It also includes an implementation, RemoteConfig, which implements the retrieval of config from the Embrace Config service. This is only available when an Embrace appId is present.

github-actions · 2024-09-06T19:50:16Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

github-actions · 2024-09-06T20:08:40Z

	Warnings
⚠️	No CHANGELOG entry added.
⚠️	Sources/EmbraceCore/Embrace.swift#L261 - Files should have a single trailing newline (`trailing_newline`)
⚠️	Tests/TestSupport/TestConstants.swift#L13 - Prefer non-optional `Data(_:)` initializer when converting `String` to `Data` (`non_optional_string_data_conversion`)

Generated by 🚫 Danger Swift against 4cf5fb8

codecov · 2024-09-06T20:52:36Z

Codecov Report

Attention: Patch coverage is 92.20056% with 56 lines in your changes missing coverage. Please review.

Project coverage is 91.92%. Comparing base (fe3675e) to head (3b2d2f4).

Files with missing lines	Patch %	Lines
...lTests/EmbraceConfigurable/RemoteConfigTests.swift	83.08%	23 Missing ⚠️
...ts/TestSupport/Mocks/MockEmbraceConfigurable.swift	74.57%	15 Missing ⚠️
Sources/EmbraceConfigInternal/EmbraceConfig.swift	85.18%	4 Missing ⚠️
...figuration/EmbraceConfigurable/DefaultConfig.swift	75.00%	3 Missing ⚠️
...braceConfiguration/NetworkPayloadCaptureRule.swift	78.57%	3 Missing ⚠️
Sources/EmbraceCore/Internal/Embrace+Config.swift	86.95%	3 Missing ⚠️
...figInternal/EmbraceConfigurable/RemoteConfig.swift	96.55%	2 Missing ⚠️
...urable/RemoteConfig/RemoteConfigFetcherTests.swift	98.58%	2 Missing ⚠️
Sources/EmbraceCore/Embrace.swift	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
- Coverage   91.93%   91.92%   -0.01%     
==========================================
  Files         400      408       +8     
  Lines       26024    26206     +182     
==========================================
+ Hits        23924    24091     +167     
- Misses       2100     2115      +15

Files with missing lines	Coverage Δ
...eCommonInternal/Identifiers/DeviceIdentifier.swift	`95.45% <100.00%> (+62.12%)`	⬆️
.../EmbraceConfigInternal/EmbraceConfig+Options.swift	`100.00% <100.00%> (ø)`
...nfigurable/RemoteConfig/RemoteConfig+Options.swift	`100.00% <100.00%> (ø)`
...onfigurable/RemoteConfig/RemoteConfigFetcher.swift	`91.01% <100.00%> (ø)`
...onfigurable/RemoteConfig/RemoteConfigPayload.swift	`100.00% <ø> (ø)`
...urces/EmbraceConfiguration/InternalLogLimits.swift	`100.00% <100.00%> (ø)`
...kPayloadCapture/NetworkPayloadCaptureHandler.swift	`91.89% <ø> (ø)`
...workPayloadCapture/URLSessionTaskCaptureRule.swift	`88.63% <ø> (ø)`
...raceCore/Internal/Logs/DefaultInternalLogger.swift	`87.50% <100.00%> (+4.76%)`	⬆️
Sources/EmbraceCore/Options/Embrace+Options.swift	`84.00% <100.00%> (+0.66%)`	⬆️
... and 17 more

... and 4 files with indirect coverage changes

Sources/EmbraceConfigInternal/EmbraceConfigurable/EmbraceConfigurable.swift

NachoEmbrace

I'm a bit confused on the refactor, some of the changes seem unnecessary, but I might be missing some context.
My understanding was that we were going to add a protocol and make EmbraceConfig follow that, but this is a bit more than that.

NachoEmbrace · 2024-09-11T20:26:41Z

Sources/EmbraceConfigInternal/EmbraceConfig.swift

+    public init(
+        configurable: EmbraceConfigurable,
+        options: Options,
+        notificationCenter: NotificationCenter,
+        logger: InternalLogger
+    ) {


The purpose of this class is not clear now. It just contains a EmbraceConfigurable and just passes through the callbacks.
Unless I'm missing something this class is not really doing anything, so I think it should be removed or rolled back to what it originally was (the remote config implementation). From what I see the new RemoteConfig is basically what this class was before.

So maybe just rename it to EmbraceRemoteConfig and remove the new one?
Or just remove this class and keep the new one?
Both paths basically lead to the same result.

EmbraceConfig has 2 main functions, hold on to the underlying config implementation, and provide the "updateIfNeeded" logic such that config is only updated based on the minimumUpdateInterval.

This feels more like a "Manager" type of class because of this change.

I see that this class has the minimumUpdateInterval logic but I don't understand why it had to be split into two classes still.

The alternative would be to require the object that conforms to EmbraceConfigurable to provide this behavior. I felt it would be best if the SDK defined this so that it was consistent across all conformers.

Another piece is the "config did update" notification. I didn't want to push that requirement onto the EmbraceConfigurable as well without having an explicit interface for it.

Sources/EmbraceConfigInternal/EmbraceConfig.swift

NachoEmbrace · 2024-09-11T20:28:03Z

Sources/EmbraceConfigInternal/EmbraceConfigurable/EmbraceConfigurable.swift

+
+    var isNetworkSpansForwardingEnabled: Bool { get }
+
+    var internalLogLimits: InternalLogLimits { get }


Do we really need internal log limits in the public definition of the config?

I wanted to make this interface as concise as possible. Instead of getters for all the severities, I went with exposing the InternalLogLimits object directly.

We may want to rename InternalLogLimits to differentiate it from the Internal target suffix we use.

What I mean is "should internal log limits even be visible in this protocol or just remain as a complete internal thing in our config module?".
Even though these logs end up as otel logs, meaning anyone setting up an exporter will see them, I'm not sure we should have them definable in the injected config. I don't see why people would need this.

This config determines if these logs are emitted. I think its ok if a client determines they don't want these logs alongside their data.

NachoEmbrace · 2024-09-11T20:30:50Z

Sources/EmbraceConfigInternal/EmbraceConfigurable/DefaultConfig.swift

+public class DefaultConfig: EmbraceConfigurable {
+    public let isSDKEnabled: Bool = true
+
+    public let isBackgroundSessionEnabled: Bool = false
+
+    public let isNetworkSpansForwardingEnabled: Bool = false
+
+    public let internalLogLimits = InternalLogLimits()
+
+    public let networkPayloadCaptureRules = [NetworkPayloadCaptureRule]()
+
+    public func update() { /* No op */ }
+
+    public init() { }
+}


I think having an explicit declaration of the default config is good, but right now this is duplicating functionality in RemoteConfigPayload (aka default values are being set in both classes).

We should find a way to not duplicate those constants.

I think the duplication is ok. This is a separate implementation of the EmbraceConfigurable protocol. The initial values in the other implementation, RemoteConfig, could be different.

Was this defined anywhere? (I mean the potential difference in the default behavior of the SDK when there's an app id and where there isn't).
I don't think we would want those behaviors to be different ever but I could be wrong.

Sources/EmbraceConfigInternal/EmbraceConfigurable/RemoteConfig.swift

Sources/EmbraceConfigInternal/EmbraceConfigurable/RemoteConfig/RemoteConfigFetcher.swift

…otocol with different implementations, RemoteConfig and DefaultConfig

Pulls responsibility from EmbraceConfigTests

Also tests logic around enabling config based on device_id + configured threshold

…and delegation to underlying EmbraceConfigurable implementation

This is separate from `EmbraceConfigInternal` (it is a dependency there) This target is meant for those that would like to inject an `EmbraceConfigurable` implementation into the `Embrace.Options` when setting up the Embrace instance.

…ect.swift, and podspec template

This is used to post `embraceConfigUpdated` notification

…nal RemoteConfigFetcher

Sources/EmbraceConfigInternal/EmbraceConfigurable/RemoteConfig/RemoteConfigFetcher.swift

…/RemoteConfigFetcher.swift Co-authored-by: ArielDemarco <[email protected]>

ArielDemarco · 2024-10-01T18:58:14Z

Sources/EmbraceConfiguration/EmbraceConfigurable.swift

+    ///     - completion: A completion block that takes two parameters (didChange, error). Completion block should pass `true` 
+    ///     if the configuration now has different values and `false` if not in the case of an error updating, the completion block should
+    ///     return `false` and an Error object describing the issue.
+    func update(completion: @escaping (Bool, Error?) -> Void)


Wouldn't be cleaner something like Result<Bool, Error>. In that case, it’s easier to understand that:

.success will be either true (was updated) or false (wasn't updated).

.failure will always mean that something went wrong (and obviously, an update couldn't be done).
As far as I can see, the remote and default config would work perfectly fine with this typification. Also, this approach prevents a peculiar case where you could have an error, yet still indicate that the update was done successfully (which seems contradictory)

Yea I agree, the issue is that this protocol is exposed to objc and the Result enum is not available

atreat self-assigned this Sep 6, 2024

atreat force-pushed the atreat/embrace-configurable branch from 2ab597e to c7e7797 Compare September 6, 2024 19:50

atreat commented Sep 11, 2024

View reviewed changes

Sources/EmbraceConfigInternal/EmbraceConfigurable/EmbraceConfigurable.swift Outdated Show resolved Hide resolved

atreat marked this pull request as ready for review September 11, 2024 19:04

atreat requested a review from a team as a code owner September 11, 2024 19:04

NachoEmbrace reviewed Sep 11, 2024

View reviewed changes

atreat changed the title ~~Adds Embrace configurable. Allows Option to be passed in when app_id not present to override EmbraceConfig behavior~~ Adds EmbraceConfigurable. Allows Option to be passed in when app_id not present to override EmbraceConfig behavior Sep 16, 2024

atreat force-pushed the atreat/embrace-configurable branch from 5aeb6e4 to 0d1c169 Compare September 16, 2024 16:07

atreat added 10 commits September 26, 2024 15:36

WIP - Initial refactor of config to separate EmbraceConfigurable pr…

a0db855

…otocol with different implementations, RemoteConfig and DefaultConfig

Updates RemoteConfigFetcherTests

97570d2

Pulls responsibility from EmbraceConfigTests

Adds RemoteConfigTests to test EmbraceConfigurable implementation

8e84602

Also tests logic around enabling config based on device_id + configured threshold

Updates RemoteConfigTests with addition of NetworkPayloadCaptureRule

8e46e92

Adds tests and updates docC

cb1ac17

Adds updated EmbraceConfigTests to check for updateIfNeeded behavior …

bed0560

…and delegation to underlying EmbraceConfigurable implementation

Creates new target EmbraceConfiguration.

b6c6647

This is separate from `EmbraceConfigInternal` (it is a dependency there) This target is meant for those that would like to inject an `EmbraceConfigurable` implementation into the `Embrace.Options` when setting up the Embrace instance.

Adds EmbraceConfiguration target to build_xcframeworks script, Proj…

975bb08

…ect.swift, and podspec template

Updates EmbraceConfigurable.update method to take completion handler

d9d5e79

This is used to post `embraceConfigUpdated` notification

Updates RemoteConfig to have public initializer not require the inter…

3b2d2f4

…nal RemoteConfigFetcher

atreat force-pushed the atreat/embrace-configurable branch from 0945988 to 3b2d2f4 Compare September 26, 2024 19:37

ArielDemarco reviewed Oct 1, 2024

View reviewed changes

Sources/EmbraceConfigInternal/EmbraceConfigurable/RemoteConfig/RemoteConfigFetcher.swift Outdated Show resolved Hide resolved

Update Sources/EmbraceConfigInternal/EmbraceConfigurable/RemoteConfig…

8a4225a

…/RemoteConfigFetcher.swift Co-authored-by: ArielDemarco <[email protected]>

ArielDemarco reviewed Oct 1, 2024

View reviewed changes

ArielDemarco approved these changes Oct 1, 2024

View reviewed changes

atreat added 3 commits October 2, 2024 13:56

Fixes tests after change to query items

8fbb200

Merge branch 'main' into atreat/embrace-configurable

3a52c52

Updates SessionControllerTests after merge from main

4cf5fb8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds EmbraceConfigurable. Allows Option to be passed in when app_id not present to override EmbraceConfig behavior #56

Adds EmbraceConfigurable. Allows Option to be passed in when app_id not present to override EmbraceConfig behavior #56

atreat commented Sep 6, 2024

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading

NachoEmbrace left a comment

NachoEmbrace Sep 11, 2024 •

edited

Loading

atreat Sep 11, 2024

NachoEmbrace Sep 12, 2024

atreat Sep 23, 2024

NachoEmbrace Sep 11, 2024

atreat Sep 11, 2024

NachoEmbrace Sep 12, 2024 •

edited

Loading

atreat Sep 23, 2024

NachoEmbrace Sep 11, 2024

atreat Sep 11, 2024

NachoEmbrace Sep 12, 2024

ArielDemarco Oct 1, 2024

atreat Oct 2, 2024


		var isNetworkSpansForwardingEnabled: Bool { get }

		var internalLogLimits: InternalLogLimits { get }

Adds EmbraceConfigurable. Allows Option to be passed in when app_id not present to override EmbraceConfig behavior #56

Are you sure you want to change the base?

Adds EmbraceConfigurable. Allows Option to be passed in when app_id not present to override EmbraceConfig behavior #56

Conversation

atreat commented Sep 6, 2024

github-actions bot commented Sep 6, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

github-actions bot commented Sep 6, 2024 • edited Loading

codecov bot commented Sep 6, 2024 • edited Loading

Codecov Report

NachoEmbrace left a comment

Choose a reason for hiding this comment

NachoEmbrace Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NachoEmbrace Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading

NachoEmbrace Sep 11, 2024 •

edited

Loading

NachoEmbrace Sep 12, 2024 •

edited

Loading