CCM-13304: Generate reports #203
Conversation
| apim_api_key_ssm_parameter_name = "/${var.component}/${var.environment}/apim/api_key" | ||
| apim_keystore_s3_bucket = "nhs-${var.aws_account_id}-${var.region}-${var.environment}-${var.component}-static-assets" | ||
| apim_private_key_ssm_parameter_name = "/${var.component}/${var.environment}/apim/private_key" | ||
| athena_reporting_database = "${local.csi}-reporting" |
There was a problem hiding this comment.
I think instead of this local we should just reference the name of the DB from glue_catalog_database_reporting.tf file. E.g. use aws_glue_catalog_database.reporting.name instead of local.athena_reporting_database.
| variable "athena_query_max_polling_attemps" { | ||
| type = number | ||
| description = "The number of times athena will be polled to check if a query is completed" | ||
| default = 50 |
There was a problem hiding this comment.
Is 50 attempts not a bit high?
| export type DataRepositoryDependencies = { | ||
| athenaClient: AthenaClient; | ||
| config: Record<string, string>; | ||
| logger: Logger; | ||
| }; |
There was a problem hiding this comment.
This type doesn't appear to be used.
Also, I think we'd be better making config an object with specific named properties, rather than a generic record. Then we can rely on the compiler to ensure the correct arguments have been passed.
| export const createStorageRepository = ({ | ||
| logger, | ||
| reportingBucketName, | ||
| s3Client, | ||
| }: StorageRepositoryDependencies): IStorageRepository => ({ | ||
| async publishReport(reportQueryId: string, reportFilePath: string) { | ||
| logger.debug( | ||
| `Publishing report data to ${reportFilePath} for query ${reportQueryId}`, | ||
| ); | ||
|
|
||
| const copyObjectCommand = new CopyObjectCommand({ | ||
| CopySource: `${reportingBucketName}/athena-output/${reportQueryId}.csv`, | ||
| Bucket: reportingBucketName, | ||
| Key: reportFilePath, | ||
| }); | ||
|
|
||
| await s3Client.send(copyObjectCommand); | ||
|
|
||
| logger.info(`Report stored at ${reportingBucketName}/${reportFilePath}.`); | ||
|
|
||
| return `s3://${reportingBucketName}/${reportFilePath}`; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
I think this could be a class, for consistency with the report service and data repository, rather than a function that returns a function.
| private async poll( | ||
| queryId: string, | ||
| maxPollLimit: number, | ||
| waitForInSeconds: number, | ||
| ) { | ||
| let count = 0; | ||
| let status = 'QUEUED'; | ||
|
|
||
| while ( | ||
| count < maxPollLimit && | ||
| ['QUEUED', 'RUNNING', 'UNKNOWN'].includes(status) | ||
| ) { | ||
| status = (await this.dataRepository.getQueryStatus(queryId)) || 'UNKNOWN'; | ||
|
|
||
| count += 1; | ||
|
|
||
| await sleep(waitForInSeconds); | ||
| } | ||
|
|
||
| return status; | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we not use util-retry for this, rather than implementing our own logic here?
| if (submittedFailedEvents.length > 0) { | ||
| logger.warn({ | ||
| description: 'Some successful events failed to publish', | ||
| failedCount: submittedFailedEvents.length, | ||
| totalAttempted: successfulItems.length, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.warn({ | ||
| err: error, | ||
| description: 'Failed to send successful events to EventBridge', | ||
| eventCount: successfulItems.length, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Should we be DLQing the message (directly, so we don't retry it) if the event publish fails? I know we're not actually doing anything with the event that's published at the moment, but I think that's the pattern we've used elsewhere, and I think as a general rule the component publishing the event shouldn't necessarily be aware of what's consuming it (so we should treat this as if an event in use has failed). I think adding the incoming event to a DLQ allows us to examine it and determine whether we need to trigger something else downstream, etc.
| CASE | ||
| WHEN e.type LIKE '%.item.dequeued.%' | ||
| OR e.type LIKE '%.item.removed.%' THEN 'Digital' | ||
| WHEN e.type LIKE '%.print.letter.transitioned.%' THEN 'Print' ELSE NULL | ||
| END as communicationtype, | ||
| CASE | ||
| WHEN e.type LIKE '%.item.dequeued.%' THEN 'Unread' | ||
| WHEN e.type LIKE '%.item.removed.%' THEN 'Read' | ||
| WHEN e.letterstatus = 'REJECTED' THEN 'Rejected' | ||
| WHEN e.letterstatus = 'FAILED' THEN 'Failed' | ||
| WHEN e.letterstatus = 'DISPATCHED' THEN 'Dispatched' | ||
| WHEN e.letterstatus = 'REJECTED' THEN 'Rejected' ELSE NULL |
There was a problem hiding this comment.
I think this will need to be updated to get the print status from the PrintLetterTransitioned event (which should now have its status field recorded in the event_record table), rather than the print.letter.transitioned.* events it's currently expecting.
2 participants
Description
Generate report component.
Testing
Generate Report Event
Report Generated in S3
Report Generated Event
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.