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

feat(platform-core): Add TypeScript definitions for CLI telemetry data v2 #2300

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

vigy02
Copy link
Contributor

@vigy02 vigy02 commented Dec 5, 2024

Problem

Adding types for the new CLI telemetry data structure to support improved usage data capturing and analysis.

Changes

  • Added new TypeScript type definitions in packages/platform-core/src/usage-data/cli_telemetry_data.ts
  • Implemented the following type structures:
    • CoreIdentifiersDetails: Contains session, timestamp, and project identification
    • CommandDetails: Captures command execution information
    • EnvironmentDetails: System environment information
    • Latency: Optional timing metrics
    • ErrorDetails: Error tracking structure
    • ProjectDetails: Project dependency information
    • AmplifyCliTelemetryData: Main type structure combining all components

Validation

The changes have been validated through:

  • Successful lint checks
  • Type compatibility verification

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

changeset-bot bot commented Dec 5, 2024

🦋 Changeset detected

Latest commit: c65d304

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/platform-core Patch

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

@vigy02 vigy02 marked this pull request as ready for review December 5, 2024 23:29
@vigy02 vigy02 requested a review from a team as a code owner December 5, 2024 23:29
path: string[];
parameters: string[];
};
executionTime: number;
Copy link
Member

Choose a reason for hiding this comment

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

I would move executionTime under Latency and call it total or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed it

Comment on lines 41 to 55
// Error Details (Optional)
export type ErrorDetails = {
primary: {
name: string;
message: string;
};
cause?: Array<{
name: string;
message: string;
methodName: string;
file: string;
lineNumber: number;
columnNumber: string;
}>;
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Error Details (Optional)
export type ErrorDetails = {
primary: {
name: string;
message: string;
};
cause?: Array<{
name: string;
message: string;
methodName: string;
file: string;
lineNumber: number;
columnNumber: string;
}>;
};
// Error Details (Optional)
export type ErrorDetails = {
name: string;
message: string;
stack: string;
cause?: ErrorDetails;
};

@Amplifiyer wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, that's what I had in mind. We need to validate this with a POC though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, it !

Will look something like this
export type ErrorDetails = {
name: string;
message: string;
stack: string;
cause?: ErrorDetails;
};

const errorDetails: ErrorDetails = {
name: "Error",
message: "API layer error",
stack: Error: API layer error at throwLevel3 (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:113:15) at TestContext.<anonymous> (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:129:7) at async Test.run (node:internal/test_runner/test:632:9) at async Suite.processPendingSubtests (node:internal/test_runner/test:374:7),
cause: {
name: "Error",
message: "Service layer error",
stack: Error: Service layer error at throwLevel2 (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:106:15) at throwLevel3 (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:111:9) at TestContext.<anonymous> (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:129:7) at async Test.run (node:internal/test_runner/test:632:9) at async Suite.processPendingSubtests (node:internal/test_runner/test:374:7),
cause: {
name: "Error",
message: "Data layer error",
stack: Error: Data layer error at throwLevel1 (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:99:15) at throwLevel2 (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:104:9) at throwLevel3 (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:111:9) at TestContext.<anonymous> (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:129:7) at async Test.run (node:internal/test_runner/test:632:9) at async Suite.processPendingSubtests (node:internal/test_runner/test:374:7),
cause: {
name: "Error",
message: "Database connection failed",
stack: Error: Database connection failed at throwLevel1 (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:97:15) at throwLevel2 (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:104:9) at throwLevel3 (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:111:9) at TestContext.<anonymous> (/Users/vigy/Documents/GitHub/amplify-backend/packages/cli-core/src/printer/printer.test.ts:129:7) at async Test.run (node:internal/test_runner/test:632:9) at async Suite.processPendingSubtests (node:internal/test_runner/test:374:7)
}
}
}
};

localProjectId: string;
accountId?: string;
payloadVersion: string;
awsRegion: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

The region would be optional as well. E.g. apmx configure profile wouldn't have a region when the command is executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it

};

// Command Details
export type CommandDetails = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to call this something other than Command such as Event that we are interested in. In most cases it's a command execution, in other it could be an API invocation or in more usual case, a sandbox file change event that triggers a deployment.

Then we can have an eventId as well. Such that sessionId remains the same for all deployments happened through a sandbox invocation but each with different eventId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice touch, I have added this

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.

3 participants