Skip to content

VED 292 single record exception mgt#459

Closed
nhsdevws wants to merge 9 commits intomasterfrom
VED-292-Single-Record-Exception-Mgt
Closed

VED 292 single record exception mgt#459
nhsdevws wants to merge 9 commits intomasterfrom
VED-292-Single-Record-Exception-Mgt

Conversation

@nhsdevws
Copy link
Copy Markdown
Contributor

@nhsdevws nhsdevws commented May 16, 2025 •
edited
Loading

Summary

  • Routine Change
  • ❗ Breaking Change
  • 🤖 Operational or Infrastructure Change
  • ✨ New Feature
  • ⚠️ Potential issues that might be caused by this change

https://nhsd-jira.digital.nhs.uk/browse/VED-292
Changes:

  • Refactor to process individual records in a function
  • unit tests, including to check processing continues if an error occurs
  • protect sqs initialisation from unhandled exception
  • wrap firehose with exception mgt
  • prevent unhandled exception in handler exception catch
  • Skipped records are logged to firehose, but not saved

Reviews Required

  • Dev
  • Test
  • Tech Author
  • Product Owner

Review Checklist

ℹ️ This section is to be filled in by the reviewer.

  • I have reviewed the changes in this PR and they fill all or part of the acceptance criteria of the ticket, and the code is in a mergeable state.
  • If there were infrastructure, operational, or build changes, I have made sure there is sufficient evidence that the changes will work.
  • I have ensured the changelog has been updated by the submitter, if necessary.

@nhsdevws nhsdevws self-assigned this May 16, 2025
@nhsdevws nhsdevws force-pushed the VED-292-Single-Record-Exception-Mgt branch from 32bdd4a to 714a2eb Compare May 20, 2025 11:42
@nhsdevws nhsdevws force-pushed the VED-292-Single-Record-Exception-Mgt branch from 2854009 to 05ab289 Compare May 20, 2025 12:34
mfjarvis
mfjarvis reviewed May 20, 2025
View reviewed changes
robertnovac1
robertnovac1 previously approved these changes May 20, 2025
View reviewed changes
mfjarvis
mfjarvis reviewed May 20, 2025
View reviewed changes
delta_backend/tests/test_delta.py Outdated
# Assert
self.assertFalse(result)
self.assertEqual(mock_table.put_item.call_count, 3)
self.assertEqual(self.mock_firehose_logger.send_log.call_count, 2)
Copy link
Copy Markdown
Contributor

@mfjarvis mfjarvis May 20, 2025

Choose a reason for hiding this comment

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

Do you know any reason why we don't log errors processing an individual record to firehose, only successes or failures in the overall Lambda handler?

Copy link
Copy Markdown
Contributor

@mfjarvis mfjarvis May 20, 2025

Choose a reason for hiding this comment

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

Secondary question I suppose is why we're logging to Firehose at all when we're also logging to CloudWatch, but that's definitely one for another ticket

Copy link
Copy Markdown
Contributor Author

@nhsdevws nhsdevws May 20, 2025

Choose a reason for hiding this comment

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

Firehose seems superfluous. The Data is already sent to DLQ and is logged to cloudwatch. TBH I dont know why it is there.

It also sends the error to firehose in the handler exception. If firehose has an issue, we will have an unhandled exception

Perhaps we need a ticket to look at refactoring with these comments

Copy link
Copy Markdown
Contributor Author

@nhsdevws nhsdevws May 20, 2025

Choose a reason for hiding this comment

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

ticket raised https://nhsd-jira.digital.nhs.uk/browse/VED-301

Copy link
Copy Markdown
Contributor

@mfjarvis mfjarvis May 20, 2025

Choose a reason for hiding this comment

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

As discussed - the logging change was introduced in this PR so let's fix that now. The investigation into why we're logging to Firehose can come later 👍

Copy link
Copy Markdown
Contributor Author

@nhsdevws nhsdevws May 21, 2025

Choose a reason for hiding this comment

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

All conditions now log to FH

mfjarvis
mfjarvis reviewed May 20, 2025
View reviewed changes
@nhsdevws nhsdevws requested review from mfjarvis and robertnovac1 May 20, 2025 14:54
@nhsdevws nhsdevws enabled auto-merge (squash) May 21, 2025 12:11
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented May 22, 2025

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@nhsdevws
Copy link
Copy Markdown
Contributor Author

nhsdevws commented May 23, 2025

Close PR to facilitate conflict resolution. New PR #502

@nhsdevws nhsdevws closed this May 23, 2025
auto-merge was automatically disabled May 23, 2025 08:30

Pull request was closed

@nhsdevws nhsdevws deleted the VED-292-Single-Record-Exception-Mgt branch June 6, 2025 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Akol125 Akol125 Awaiting requested review from Akol125

@mfjarvis mfjarvis Awaiting requested review from mfjarvis

@robertnovac1 robertnovac1 Awaiting requested review from robertnovac1

Assignees

@nhsdevws nhsdevws

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nhsdevws @mfjarvis @robertnovac1