feat: Add usage data metrics for sandbox #642
Conversation
🦋 Changeset detectedLatest commit: 442ffc5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const userName = userInfo().username; | ||
| const cwdPath = process.cwd(); | ||
|
|
||
| return uuidV5(cwdPath + packageJsonName + userName, namespace); |
There was a problem hiding this comment.
should this mix some "machine identifier" like mac address ?
There was a problem hiding this comment.
the network interfaces could be named differently and some have just all 0s in it. Would this be a better idea https://www.npmjs.com/package/node-machine-id?
There was a problem hiding this comment.
The network interface was an example. That library looks promising if it's maintained. So feel free to find best solution for this. We need some machine id.
There was a problem hiding this comment.
This doesn't look well supported or a good alternative that exists without requiring a local state.
There was a problem hiding this comment.
We shouldn't include cwd in this seed. You can run npx amplify from anywhere in your project. It should be the path of the root package.json or something like that
There was a problem hiding this comment.
Now I'm wondering if we should only seed with __dirname? The packageJsonName and userName could both change without actually being a different installation. __dirname should be stable for a given installation in all normal scenarios I can think of
There was a problem hiding this comment.
__dirname + some "machine identifier". (if we punt machine id we should create gh issue)
There was a problem hiding this comment.
userName changing is unlikely though it's usually is included in the path (for unix at least), however for cases where users are installing an "example" app in a /tmp or a root directory, we will get same installation ids across different users.
There is no reliable way to get machine identifier as per my quick research. We will have to punt that.
There was a problem hiding this comment.
Updated to use __dirname for now
There was a problem hiding this comment.
Long term - #665
| ) {} | ||
|
|
||
| emitSuccess = async ( | ||
| metrics?: Record<string, number>, |
There was a problem hiding this comment.
Can we have a stronger type here than just string record?
There was a problem hiding this comment.
The reasoning behind is that "metrics collector" should expose api to collect metrics (i.e. abstract mechanism of how data is pushed to the backend) but it should be up to caller to know what exactly is being reported.
There was a problem hiding this comment.
That's true, but we also have logic in here where we are looking for specific keys to perform certain transformations. Seems like we could get the type system to help us.
Regardless, I'm not going to block on it
There was a problem hiding this comment.
That's a good point. But I'd do that by casting these Records to something typed internally in data collector as we do this only because we want to keep old schema.
packages/platform-core/src/telemetry/get_usage_data_url.test.ts
Outdated
Show resolved
Hide resolved
packages/sandbox/package.json
Outdated
| }, | ||
| "devDependencies": { | ||
| "@types/debounce-promise": "^3.1.6", | ||
| "@types/parse-gitignore": "^1.0.0" | ||
| "@types/parse-gitignore": "^1.0.0", | ||
| "@types/uuid": "^9.0.7" |
There was a problem hiding this comment.
we can use crypto.randomUUID to avoid pulling in this dep
There was a problem hiding this comment.
this is actually needed in platform-core (moved there) and since it's core, we should use the browser compatible libraries and I'm not sure if node's crypto is.
| const aggregatorStderrStream = new stream.Writable(); | ||
| aggregatorStderrStream._write = function (chunk, encoding, done) { |
There was a problem hiding this comment.
I didn't realize that aggregating output is existing thing.
Mind logging GH issue to make this part "streaming and parsing errors" ?
There was a problem hiding this comment.
Will log it, this would be a little harder since we are actually throwing and processing elsewhere.
packages/cli/src/backend-identifier/local_namespace_resolver.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/backend-identifier/local_namespace_resolver.ts
Outdated
Show resolved
Hide resolved
| const userName = userInfo().username; | ||
| const modulePath = __dirname; | ||
|
|
||
| return uuidV5(modulePath + packageJsonName + userName, namespace); |
There was a problem hiding this comment.
My vote would still be for just modulePath + userName. While packageJsonName should be pretty stable, it is subject to change (we have changed our package.json names on occasion)
There was a problem hiding this comment.
I think in that case it's acceptable for installation id to change. I think it's worse to have two customers sharing the installation id vs one customer to have many. Nevertheless I'm not strong on that so changed it to remove package.json name
There was a problem hiding this comment.
Maybe for machine id we could use os.hostname() for now ? https://nodejs.org/api/os.html#oshostname
| // eslint-disable-next-line spellcheck/spell-checker | ||
| const AMPLIFY_CLI_UUID_NAMESPACE = 'e7368840-2eb6-4042-99b4-9d6c2a9370e6'; // A random v4 UUID | ||
| /** | ||
| * Generates a consistent installation Uuid from the cwd + packageJsonName + userName |
| * | ||
| * Throws if no package.json is present | ||
| */ | ||
| export class CwdPackageJsonReader { |
There was a problem hiding this comment.
I wonder if this should just be another method on PackageJsonReader. ie .readFromCwd or such?
There was a problem hiding this comment.
I'll accommodate that in a follow up PR.
Description of changes: Add usage data metrics for sandbox
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.