Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUMM-2203 Use unique data directory for each instance of the SDK #906

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Jun 21, 2022

What and why?

📦 With this PR, each instance of DatadogCore will use a different directory for managing its data.

This is a step forward towards V2 architecture, where we're going to support multiple SDK instances. Each instance of the SDK needs to persist data in different locations.

This is the content of /Library/Caches/ when using two instances of DatadogCore:

Screenshot 2022-06-22 at 17 13 47

How?

Each DatadogCore now accepts a "core directory". I introduced a separate CoreDirectory type that wraps Directory - this is to enforce type safety at configuration level and to avoid passing wrong directory by mistake.

The CoreDirectory creates the folder structure for SDK and its Features. It uses a hash function (sha256) for computing the folder name that is unique for given instance of the SDK:

let directoryName = sha256("\(clientToken)\(site)")

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests
  • Run integration tests
  • Run smoke tests

@ncreated ncreated self-assigned this Jun 21, 2022
@ncreated ncreated force-pushed the ncreated/RUMM-2203-make-sdk-root-folder-specific-to-sdk-instance branch from 454ffda to 9fcd11a Compare June 22, 2022 15:31
Comment on lines 27 to 29
/// The list of deprecated paths from previous versions of this feature. It is used to perform cleanup.
/// These paths must be relative to the known OS location (`/Library/Caches/`) containing core directories for different instances of the SDK.
let deprecated: [String]
Copy link
Member Author

@ncreated ncreated Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxep When writing this comment I realised that we cannot ship V1's data deprecation as part of V2 SDK. Because V1 uses single folder, the V2 SDK cannot know if particular com.datadoghq.<feature> folder corresponds to this or foreign installation. Think: a vendor SDK installed in an app where both use Datadog SDK but vendor uses V2 and app is on V1.

What we can do instead is having this deprecated paths work the same as authorised and unauthorized - meaning they will be applied relatively to the SDK "core directory". It implies no need for this configuration now, but we could eventually leverage it later. We could remove this setup for now, or keep it empty.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a vendor SDK installed in an app where both use Datadog SDK but vendor uses V2 and app is on V1

If both versions mismatch, the app won't be able to install the vendor framework as a dependency, except if the vendor is manually linking the Datadog SDK as a static lib, which should be discouraged IMO. The app would have to upgrade to v2 or use a previous version of the vendor SDK to match the version. So I wouldn't consider this use case.

Now, multiple instances of the v2 SDK will perform the same operation on the deprecated v1 directories, so I wonder if this is the right place for v1 depreciation 🤔 We can consider this as a temporary measure and eventually remove this deprecated when we consider good adoption for v2 (which can take time..)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Right, I think it's fair to assume there will be a single version of the SDK even if multiple instances are enabled.

Indeed, there's a room for improvement around deprecation logic. Because it requires more refactoring in different parts of the codebase, I created RUMM-2312 to focus solely on data deprecation logic. For now, I'll keep it as it is - capable of removing global SDK folders, without ensuring the logic is ran only once.

@ncreated ncreated marked this pull request as ready for review June 22, 2022 15:45
@ncreated ncreated requested a review from a team as a code owner June 22, 2022 15:45
@ncreated ncreated requested a review from maxep June 27, 2022 09:36
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work with the hash for identifying core instances 👍
LGTM ✅

@ncreated ncreated merged commit 00a3d9c into feature/v2 Jun 27, 2022
@ncreated ncreated deleted the ncreated/RUMM-2203-make-sdk-root-folder-specific-to-sdk-instance branch June 27, 2022 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants