-
Notifications
You must be signed in to change notification settings - Fork 52
Add Sentry error tracking for environment load/transform #258
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
base: master
Are you sure you want to change the base?
Changes from all commits
6bee3df
9f35636
cf5e853
dc31c45
94bcff9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||
| import { v4 as uuidv4 } from "uuid"; | ||||||
| import * as Sentry from "@sentry/browser"; | ||||||
| import { | ||||||
| API, | ||||||
| APIEntity, | ||||||
|
|
@@ -275,17 +276,35 @@ export async function writeContent( | |||||
| }; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| const parsedContentResult = parseRaw(content, fileType.validator); | ||||||
| if (parsedContentResult.type === "error") { | ||||||
| // Send analytics event for validation failure (avoid sending raw content) | ||||||
| const contentMeta = | ||||||
| typeof content === "string" | ||||||
| ? { contentLength: content.length } | ||||||
| : { keys: Object.keys(content).length }; | ||||||
|
|
||||||
| Sentry.captureEvent({ | ||||||
| message: `parseRaw() - Content validation error: ${parsedContentResult.error.message}`, | ||||||
| level: "error", | ||||||
| tags: { | ||||||
| fileType: fileType.type, | ||||||
| errorType: "validation_failed", | ||||||
| }, | ||||||
| extra: { | ||||||
| path: resource.path, | ||||||
| error: parsedContentResult.error.message, | ||||||
| contentMeta, | ||||||
| }, | ||||||
| }); | ||||||
| return createFileSystemError( | ||||||
| { message: parsedContentResult.error.message }, | ||||||
| resource.path, | ||||||
| fileType.type | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| console.log("writing at", resource.path); | ||||||
| const serializedContent = serializeContentForWriting(content); | ||||||
| if (writeWithElevatedAccess) { | ||||||
| await FsService.writeFileWithElevatedAccess( | ||||||
|
|
@@ -307,6 +326,20 @@ export async function writeContent( | |||||
| }, | ||||||
| }; | ||||||
| } catch (e: any) { | ||||||
| // Send analytics event for write exception (without raw content) | ||||||
| Sentry.captureEvent({ | ||||||
| message: `writeContent() - File write error: ${e.message}`, | ||||||
| level: "error", | ||||||
| tags: { | ||||||
| fileType: fileType.type, | ||||||
| errorType: "write_exception", | ||||||
| }, | ||||||
| extra: { | ||||||
| path: resource.path, | ||||||
| error: e.message, | ||||||
| stack: e.stack, | ||||||
| }, | ||||||
| }); | ||||||
| return createFileSystemError(e, resource.path, fileType.type); | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -1060,55 +1093,144 @@ export function sanitizeFsResourceList( | |||||
| export async function parseFileToEnv( | ||||||
| file: FileResource | ||||||
| ): Promise<FileSystemResult<Environment>> { | ||||||
| const parsedFileResult = await parseFile({ | ||||||
| resource: file, | ||||||
| fileType: new EnvironmentRecordFileType(), | ||||||
| }); | ||||||
|
|
||||||
| if (parsedFileResult.type === "error") { | ||||||
| return parsedFileResult; | ||||||
| } | ||||||
| try { | ||||||
| const parsedFileResult = await parseFile({ | ||||||
| resource: file, | ||||||
| fileType: new EnvironmentRecordFileType(), | ||||||
| }); | ||||||
|
|
||||||
| const { content } = parsedFileResult; | ||||||
| if (parsedFileResult.type === "error") { | ||||||
| Sentry.captureEvent({ | ||||||
| message: `parseFileToEnv() - Environment load failed: ${parsedFileResult.error.message}`, | ||||||
| level: "error", | ||||||
| tags: { | ||||||
| fileType: "environment", | ||||||
| errorType: "environment_load_failed", | ||||||
| }, | ||||||
| extra: { | ||||||
| path: file.path, | ||||||
| error: parsedFileResult.error.message, | ||||||
| errorCode: parsedFileResult.error.code, | ||||||
| }, | ||||||
| }); | ||||||
|
|
||||||
| const isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`); | ||||||
| const environment: Environment = { | ||||||
| type: "environment", | ||||||
| id: getIdFromPath(file.path), | ||||||
| name: content.name, | ||||||
| variables: content.variables, | ||||||
| isGlobal, | ||||||
| }; | ||||||
| return parsedFileResult; | ||||||
| } | ||||||
|
|
||||||
| const result: FileSystemResult<Environment> = { | ||||||
| type: "success", | ||||||
| content: environment, | ||||||
| }; | ||||||
| const { content } = parsedFileResult; | ||||||
| const isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross-platform path separator issue persists. This issue was flagged in a previous review but remains unaddressed. Using a hardcoded forward slash 🐛 Recommended fix- const isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`);
+ const isGlobal = path.basename(file.path) === GLOBAL_ENV_FILE;Note: The 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| const environment: Environment = { | ||||||
| type: "environment", | ||||||
| id: getIdFromPath(file.path), | ||||||
| name: content.name, | ||||||
| variables: content.variables, | ||||||
| isGlobal, | ||||||
| }; | ||||||
|
|
||||||
| return result; | ||||||
| return { | ||||||
| type: "success", | ||||||
| content: environment, | ||||||
| }; | ||||||
| } catch (e: any) { | ||||||
| Sentry.captureEvent({ | ||||||
| message: `parseFileToEnv() - Environment load exception: ${e.message}`, | ||||||
| level: "error", | ||||||
| tags: { | ||||||
| fileType: "environment", | ||||||
| errorType: "environment_load_exception", | ||||||
| }, | ||||||
| extra: { | ||||||
| path: file.path, | ||||||
| error: e.message, | ||||||
| stack: e.stack, | ||||||
| }, | ||||||
| }); | ||||||
| return createFileSystemError(e, file.path, FileTypeEnum.ENVIRONMENT); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| export function parseToEnvironmentEntity( | ||||||
| variables: Record<string, EnvironmentVariableValue> | ||||||
| ) { | ||||||
| const newVariables: Record< | ||||||
| string, | ||||||
| Static<(typeof EnvironmentRecord)["variables"]> | ||||||
| > = {}; | ||||||
| // eslint-disable-next-line | ||||||
| for (const key in variables) { | ||||||
| newVariables[key] = { | ||||||
| value: variables[key].syncValue, | ||||||
| type: | ||||||
| variables[key].type === EnvironmentVariableType.Secret | ||||||
| ? EnvironmentVariableType.String | ||||||
| : variables[key].type, | ||||||
| id: variables[key].id, | ||||||
| isSecret: variables[key].type === EnvironmentVariableType.Secret, | ||||||
| }; | ||||||
| } | ||||||
| try { | ||||||
| const newVariables: Record< | ||||||
| string, | ||||||
| Static<(typeof EnvironmentRecord)["variables"]> | ||||||
| > = {}; | ||||||
| const missingFieldsPerVariable: Record<string, string[]> = {}; | ||||||
|
|
||||||
| // eslint-disable-next-line | ||||||
| for (const key in variables) { | ||||||
| const variable = variables[key]; | ||||||
| const missingFields: string[] = []; | ||||||
|
|
||||||
| const resolvedValue = | ||||||
| variable.localValue !== undefined | ||||||
| ? variable.localValue | ||||||
| : variable.syncValue !== undefined | ||||||
| ? variable.syncValue | ||||||
| : (variable as any).value !== undefined | ||||||
| ? (variable as any).value | ||||||
| : undefined; | ||||||
|
|
||||||
| if (variable.id === undefined || variable.id === null) { | ||||||
| missingFields.push("id"); | ||||||
| } | ||||||
| if (resolvedValue === undefined) { | ||||||
| missingFields.push("value"); | ||||||
| } | ||||||
| if (!variable.type) { | ||||||
| missingFields.push("type"); | ||||||
| } | ||||||
|
|
||||||
| if (missingFields.length > 0) { | ||||||
| missingFieldsPerVariable[key] = missingFields; | ||||||
| } | ||||||
|
|
||||||
| newVariables[key] = { | ||||||
| value: resolvedValue, | ||||||
| type: | ||||||
| variable.type === EnvironmentVariableType.Secret | ||||||
| ? EnvironmentVariableType.String | ||||||
| : variable.type, | ||||||
| id: variable.id, | ||||||
| isSecret: variable.type === EnvironmentVariableType.Secret, | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| return newVariables; | ||||||
| // If any variables have missing fields, send analytics event | ||||||
| if (Object.keys(missingFieldsPerVariable).length > 0) { | ||||||
| Sentry.captureEvent({ | ||||||
| message: `parseToEnvironmentEntity() - variables with missing fields: ${Object.keys(missingFieldsPerVariable).length}`, | ||||||
| level: "warning", | ||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| tags: { | ||||||
| fileType: "environment", | ||||||
| errorType: "invalid_variable_structure", | ||||||
| }, | ||||||
| extra: { | ||||||
| totalVariables: Object.keys(variables).length, | ||||||
| variablesWithIssues: Object.keys(missingFieldsPerVariable).length, | ||||||
| missingFields: missingFieldsPerVariable, | ||||||
| }, | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| return newVariables; | ||||||
| } catch (e: any) { | ||||||
| Sentry.captureEvent({ | ||||||
| message: `parseToEnvironmentEntity() - Environment variable transform exception: ${e.message}`, | ||||||
| level: "error", | ||||||
| tags: { | ||||||
| fileType: "environment", | ||||||
| errorType: "variable_transform_exception", | ||||||
| }, | ||||||
| extra: { | ||||||
| error: e.message, | ||||||
| stack: e.stack, | ||||||
| }, | ||||||
| }); | ||||||
| throw e; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| export function getFileNameFromPath(filePath: 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.
Misleading Sentry message - "not found" vs actual error type.
The message "Environment not found" is inaccurate. The error could be a validation failure, parse error, or permission issue - not necessarily a missing file. The actual error code is available via
parsedContentResult.error.codebut the message doesn't reflect it.✏️ Suggested fix
Sentry.captureEvent({ - message: `Environment not found: ${parsedFileResult.error.message}`, + message: `Environment load failed: ${parsedFileResult.error.message}`, level: "error",📝 Committable suggestion
🤖 Prompt for AI Agents