-
Notifications
You must be signed in to change notification settings - Fork 72
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: c65d304 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
path: string[]; | ||
parameters: string[]; | ||
}; | ||
executionTime: number; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it
// 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; | ||
}>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Problem
Adding types for the new CLI telemetry data structure to support improved usage data capturing and analysis.
Changes
packages/platform-core/src/usage-data/cli_telemetry_data.ts
Validation
The changes have been validated through:
Checklist
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.