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

Update CostAndUsageReports.ts with try/catch error handling to enable unknown data to be gracefully handled #1226

Open
wants to merge 1 commit into
base: changeset-release/trunk
Choose a base branch
from

Conversation

bendemora
Copy link

@bendemora bendemora commented Aug 24, 2023

Within the getEstimatesFromInputData() function (line 156 onwards):

1: Wrapped the processing of each row (both from inputData and unknownRows) inside a try-catch block.
2: In case of an error, log the problematic row along with the error message to the console.

Description of Change

Write a brief but concise description of the changes you have made while also making sure to link the relevant issue number to this PR ( e.g. fixes #ISSUE-NUMBER - optional short description)

fixes ISSUE 1223

Checklist

  • PR description included and stakeholders cc'd
  • tests are changed or added
  • yarn test passes
  • yarn lint has been run
  • git pre-commit hook is successfully executed.
    (Please refrain from using --no-verify)
  • yarn changeset was run and relevant packages have been updated as a major/minor/patch bump with descriptions
    (Determine version update using Semantic Versioning)
  • relevant documentation is changed or added

Notes

Additional information relevant to the changes

© 2021 Thoughtworks, Inc.

1: Wrapped the processing of each row (both from inputData and unknownRows) inside a try-catch block.
2: In case of an error, log the problematic row along with the error message to the console.
@bendemora bendemora requested a review from a team as a code owner August 24, 2023 20:55
@github-actions github-actions bot force-pushed the changeset-release/trunk branch 5 times, most recently from af210f5 to ddd42ec Compare August 28, 2023 19:49
Copy link
Collaborator

@camcash17 camcash17 left a comment

Choose a reason for hiding this comment

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

I think this is a great start @bendemora! i have a few questions on the location of the error handling itself and if the issue you are facing would actually lead to the catch blocks.

Also as an FYI - currently this PR is aiming to merge into cloud-carbon-footprint:changeset-release/trunk. This should just be cloud-carbon-footprint:trunk.

Thank you for taking the time to create this PR!

@@ -156,58 +156,67 @@ export default class CostAndUsageReports {
getEstimatesFromInputData(
inputData: LookupTableInput[],
): LookupTableOutput[] {
const result: LookupTableOutput[] = []
const unknownRows: CostAndUsageReportsRow[] = []
const result: LookupTableOutput[] = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can undo these changes where you add the semi-colons. It may not play well with the linter

co2e: footprintEstimate.co2e,
});
}
} catch (error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you confirm that this will in fact reach the catch block? i am not sure that the issue here would actually trigger an error, however, I may be wrong.

kilowattHours: footprintEstimate.kilowattHours,
co2e: footprintEstimate.co2e,
});
} catch (error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thought as above. I think if the error we are trying to fix is the NaN issue, we may be better to error handle at the root where the arithmetic occurs to aggregate the results. I could also see this potentially being console warnings.

@github-actions github-actions bot force-pushed the changeset-release/trunk branch 9 times, most recently from 3d6575f to edcf5af Compare September 13, 2023 03:40
@camcash17
Copy link
Collaborator

Hi @bendemora have you had any chance to look at the requested changes / questions?

@bendemora
Copy link
Author

bendemora commented Oct 31, 2023 via email

@github-actions github-actions bot force-pushed the changeset-release/trunk branch 7 times, most recently from 1af795e to dacc958 Compare November 9, 2023 21:29
@github-actions github-actions bot force-pushed the changeset-release/trunk branch 7 times, most recently from ffcd379 to 40bb0d7 Compare February 6, 2024 22:19
@github-actions github-actions bot force-pushed the changeset-release/trunk branch 2 times, most recently from 25bc544 to 083e8ea Compare February 7, 2024 20:58
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.

None yet

2 participants