☁️ Add `analytics` option to `Config.Cloud` to enable sending analytics event to cloud backend #3547

danieleformichelli · 2021-10-07T06:43:31Z

Resolves #3527

Short description 📝

Describe here the purpose of your PR.

Checklist ✅

The code architecture and patterns are consistent with the rest of the codebase.
The changes have been tested following the documented guidelines.
The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
In case the PR introduces changes that affect users, the documentation has been updated.

fortmarek

Small nits but otherwise looks good to me. However, I am not extremely familiar with this area, so I'd definitely wait for someone else also having a look!

Sources/TuistAnalytics/TuistAnalytics.swift

Tests/TuistAnalyticsTests/TuistAnalyticsDispatcherTests.swift

pepicrft

This is awesome @danyf90. Thanks for making it happen. As part of Tuist Cloud we are planning to have a home dashboard where I think we can present the data sent by this logic.

Package.resolved

pepicrft · 2021-10-12T10:14:21Z

Sources/TuistAnalytics/Resources/CloudAnalyticsResourceFactory.swift

+        self.cloudConfig = cloudConfig
+    }
+
+    func storeResource(encodedCommandEvent: Data) -> CloudStoreResource {


Nitpick:

I'd rename the method to create. The resource is already in the name of the factory and create aligns more with API conventions.

I'd turn Data into a serializable object that matches the schema expected don't the API side. If the strong contract between the client and the server becomes too limiting, we can figure out ways to add more flexibility.

I'd rename CloudStoreResource to CloudAnalyticsCreateResource.

Moreover, I'd add a unit test to ensure the resource that we get from the factory is the expected one.

Not sure about the naming, we are not really creating anything here 🤔
I have used store for consistency with the cache API, but maybe in this case send is more appropriate?

pepicrft · 2021-10-12T10:18:10Z

Sources/TuistAnalytics/TuistAnalytics.swift


 public final class TuistAnalytics {
-    public static func bootstrap() {
-        AsyncQueue.sharedInstance.register(dispatcher: TuistAnalyticsDispatcher())
+    public static func bootstrap(configLoader: ConfigLoader = ConfigLoader(manifestLoader: ManifestLoader())) throws {


To be consistent with the rest of the codebase, I'd avoid loading the configuration as part of bootstrapping analytics. If the bootstrapping of analytics requires cloudReporting: Bool, let's make that partof the method's signature and pass it from outside:

public static bootstrap(cloudReporting: Bool)

The command's service is the one loading the config and passing it down to the components that need it.

Makes sense, but this part is common across all commands, and hence bootstrapped directly in the main.swift. Would you put this logic there?

I'd create another static method on this component such as loadCloudConfig that would handle that logic. It's not ideal but I agree with the expectation of boostrap() not containing config loading. I'd then call this method explicitly in main.swift

Sources/TuistGraph/Models/Cloud.swift

pepicrft · 2021-10-12T10:20:18Z

Sources/tuist/main.swift

@@ -6,7 +6,7 @@ import TuistSupport
 if CommandLine.arguments.contains("--verbose") { try? ProcessEnv.setVar(Constants.EnvironmentVariables.verbose, value: "true") }

 TuistSupport.LogOutput.bootstrap()
-TuistAnalytics.bootstrap()
+try TuistAnalytics.bootstrap()


I see why the bootstrap business logic was modified to read the config. What about moving this to the commands?

You mean adding TuistAnalytics.bootstrap() to each command?
Given we know it is for each command it would be better to handle it upfront, avoiding duplications and forgetting to add it in some commands

Sources/TuistAnalytics/TuistAnalyticsDispatcher.swift

…lytics event to cloud backend

Sources/TuistAnalytics/TuistAnalyticsDispatcher.swift

pepicrft · 2021-10-16T16:46:01Z

👋🏼 I pushed some tests. I slept over the idea of loading the config as part of initializing analytics, and I think it's a bad idea for user experience because loading the config invokes the compiler, and that might take some time if the cache is not warmed. I'm pondering the idea of using environment variables instead for passing the cloud configuration:

TUIST_CONFIG_CLOUD_URL = ...
TUIST_CONFIG_CLOUD_PROJECT_ID = ...

Because we'd like developers to have those as part of the project, I'm thinking about having a .env file under Tuist/. It's a widespread convention in other ecosystems. Because we don't need to invoke the compiler to read that file, we can keep the loading logic as it is, but without having to read the config file, only reading the .env file.

Thoughts @danyf90 / @fortmarek ?

pepicrft · 2021-10-17T18:05:41Z

I gave another thought, and I think we can keep things as they are, but turn the Config.swift manifest into another format that can be loaded and parsed fast. For example, .yaml or .toml. The only benefit we'd lose is the experience of writing them using Xcode. I think it's a good long-term trade-off that will slightly improve the execution time of commands.

danieleformichelli · 2021-10-18T07:02:36Z

I gave another thought, and I think we can keep things as they are, but turn the Config.swift manifest into another format that can be loaded and parsed fast. For example, .yaml or .toml. The only benefit we'd lose is the experience of writing them using Xcode. I think it's a good long-term trade-off that will slightly improve the execution time of commands.

@pepibumur Isn't JSON enough for our use case, and easier to parse from Swift?

We might even go hybrid and have the logic that:

when reading commands, it reads from Config.json
when tuist editing, deserializes the json to a swift to easily edit in Xcode, and on save it serializes back to JSON

Apart from the above, I would expect the parsing of Config.swift should be quick anyway, do we have any benchmark on it? Also, it will be parsed anyway at some point of the command logic, maybe we can reuse the one parsed for analytics, and "gain" back the time we have spent earlier?

fortmarek · 2021-10-18T07:19:53Z

when tuist editing, deserializes the json to a swift to easily edit in Xcode, and on save it serializes back to JSON

The issue with this is you can edit the file without using tuist edit. But I'd also look into how much time we will save here by not compiling Config.swift to make sure the trade-off is worth it.

danieleformichelli · 2021-10-18T11:31:24Z

when tuist editing, deserializes the json to a swift to easily edit in Xcode, and on save it serializes back to JSON

Another issue is that you would lose any "executable code" you have in the swift file and the next tuist edit would start with the plain file, which is probably not acceptable 🤔

If the performance problem is not too big I would probably address it separately from this PR, what do you think @fortmarek and @pepibumur ?

pepicrft · 2021-10-18T11:42:56Z

@pepibumur Isn't JSON enough for our use case, and easier to parse from Swift?

JSON and YAML can get very messy and therefore I propose using .toml instead because it's more human-readable. We load the content in memory and parse it

The issue with this is you can edit the file without using tuist edit. But I'd also look into how much time we will save here by not compiling Config.swift to make sure the trade-off is worth it.

If you have numbers that'd be great but considering it's invoking the compiler and the dynamic linker, I doubt it's under 300 ms.

Another issue is that you would lose any "executable code" you have in the swift file and the next tuist edit would start with the plain file, which is probably not acceptable 🤔

You are right. Choosing .swift for configuring Tuist, was a natural step after using Swift for the other files. However, it was never intended to be used like other manifests (i.e. writing executable code or sharing content through helpers). I'll open a Pr documenting that we discourage that in the Config.swift file.

If the performance problem is not too big I would probably address it separately from this PR, what do you think @fortmarek and @pepibumur ?

Let's refrain from merging it until we address the other concern. If we merge and introduce a performance regression we might be worsening the DX using the tool, and the user experience should always be first when prioritizing.

danieleformichelli · 2021-10-18T12:50:42Z

@fortmarek / @pepibumur I have pushed some changes to share the ConfigLoader between the analytics bootstrap and the command execution, which should mitigate the problem of having to parse the config there.

Let me know what you think about it 🙏

pepicrft · 2021-10-18T13:25:03Z

@fortmarek / @pepibumur I have pushed some changes to share the ConfigLoader between the analytics bootstrap and the command execution, which should mitigate the problem of having to parse the config there.

Turning the config loader into a singleton is not necessary because the manifest loader is already caching the JSON-representation of the manifests (plus it makes the instance a place for storing state that might make some code flows non-deterministic).
The numbers I get for cold manifest loading are around 0.22 seconds, which is reasonable. Thefere, let's revert the ConfigLoader.shared and stick to the ConfigLoader() constructor. If CI is green after that we can merge the PR.

danieleformichelli · 2021-10-18T14:08:47Z

Done ✅ 🤞

# Conflicts: # CHANGELOG.md

- A race between 2 PRs caused build failures on `main` - #3547 - #3568 - Adding missing properties to fix compilation errors Test Plan: - Verify the project compiles (when building all targets)

fortmarek approved these changes Oct 12, 2021

View reviewed changes

Sources/TuistAnalytics/TuistAnalytics.swift Outdated Show resolved Hide resolved

Tests/TuistAnalyticsTests/TuistAnalyticsDispatcherTests.swift Outdated Show resolved Hide resolved

pepicrft requested changes Oct 12, 2021

View reviewed changes

danieleformichelli added 7 commits October 15, 2021 18:16

chore: add analytics option to Config.Cloud to enable sending ana…

aa097aa

…lytics event to cloud backend

fix: fix parsing of path in tuist analytics

6338666

fix: fix order of setUp and tearDown methods

bfad536

chore: update Package.resolved

1519523

refactor: remove unused cloud insights related code

1ed1fc0

refactor: refactor TuistAnalyticsDispatcher to support multiple backends

a7b860b

refactor: rename CloudStoreResource to CloudAnalyticsStoreResource

c3bc1c0

pepicrft reviewed Oct 15, 2021

View reviewed changes

Sources/TuistAnalytics/TuistAnalyticsDispatcher.swift Outdated Show resolved Hide resolved

Some renames

d2cdcf4

pepicrft force-pushed the feature/cloud/analytics branch from eec059e to d2cdcf4 Compare October 15, 2021 16:36

Pedro Piñera added 4 commits October 15, 2021 19:25

Test CloudAnalyticsResourceFactory

99d4281

Add a test that checks the URL

f5a463e

Add some tests

a738b95

Don't bootstrap analytics when starting the process

eb2af79

fix: fix analytics backbone URL

6522599

feat: add back bootstrap of TuistAnalytics in main.swift

7c59046

danieleformichelli force-pushed the feature/cloud/analytics branch from 0308d6d to 7c59046 Compare October 18, 2021 14:07

Merge branch 'main' into feature/cloud/analytics

146d072

pepicrft enabled auto-merge (squash) October 18, 2021 15:40

danieleformichelli added 2 commits October 18, 2021 18:00

fix: compilation error

a01ce7d

Merge branch 'main' into feature/cloud/analytics

8eb6207

# Conflicts: # CHANGELOG.md

danieleformichelli requested a review from pepicrft October 19, 2021 08:27

pepicrft disabled auto-merge October 19, 2021 15:55

pepicrft merged commit b6087af into main Oct 19, 2021

pepicrft deleted the feature/cloud/analytics branch October 19, 2021 15:55

kwridan mentioned this pull request Oct 20, 2021

Fix missing isCI property in Tests #3578

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

☁️ Add `analytics` option to `Config.Cloud` to enable sending analytics event to cloud backend #3547

☁️ Add `analytics` option to `Config.Cloud` to enable sending analytics event to cloud backend #3547

danieleformichelli commented Oct 7, 2021

fortmarek left a comment

pepicrft left a comment

pepicrft Oct 12, 2021

danieleformichelli Oct 13, 2021

pepicrft Oct 12, 2021

danieleformichelli Oct 12, 2021

fortmarek Oct 13, 2021

pepicrft Oct 12, 2021

danieleformichelli Oct 12, 2021

pepicrft commented Oct 16, 2021

pepicrft commented Oct 17, 2021

danieleformichelli commented Oct 18, 2021

fortmarek commented Oct 18, 2021

danieleformichelli commented Oct 18, 2021

pepicrft commented Oct 18, 2021

danieleformichelli commented Oct 18, 2021

pepicrft commented Oct 18, 2021

danieleformichelli commented Oct 18, 2021

☁️ Add analytics option to Config.Cloud to enable sending analytics event to cloud backend #3547

☁️ Add analytics option to Config.Cloud to enable sending analytics event to cloud backend #3547

Conversation

danieleformichelli commented Oct 7, 2021

Short description 📝

Checklist ✅

fortmarek left a comment

Choose a reason for hiding this comment

pepicrft left a comment

Choose a reason for hiding this comment

pepicrft Oct 12, 2021

Choose a reason for hiding this comment

danieleformichelli Oct 13, 2021

Choose a reason for hiding this comment

pepicrft Oct 12, 2021

Choose a reason for hiding this comment

danieleformichelli Oct 12, 2021

Choose a reason for hiding this comment

fortmarek Oct 13, 2021

Choose a reason for hiding this comment

pepicrft Oct 12, 2021

Choose a reason for hiding this comment

danieleformichelli Oct 12, 2021

Choose a reason for hiding this comment

pepicrft commented Oct 16, 2021

pepicrft commented Oct 17, 2021

danieleformichelli commented Oct 18, 2021

fortmarek commented Oct 18, 2021

danieleformichelli commented Oct 18, 2021

pepicrft commented Oct 18, 2021

danieleformichelli commented Oct 18, 2021

pepicrft commented Oct 18, 2021

danieleformichelli commented Oct 18, 2021

☁️ Add `analytics` option to `Config.Cloud` to enable sending analytics event to cloud backend #3547

☁️ Add `analytics` option to `Config.Cloud` to enable sending analytics event to cloud backend #3547