Expose key metrics to cloudwatch #355
Conversation
Vlasis-Perdikidis
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
stevebux
left a comment
stevebux
left a comment
There was a problem hiding this comment.
Looks good, just a few queries
| /** | ||
| * emits metrics of successful letter updates, including the supplier and grouped by status | ||
| */ | ||
| async function emitMetics( |
There was a problem hiding this comment.
Does this function need to be async?
There was a problem hiding this comment.
I think so. Because I call it at the end, just before the lambda handler return.
So I'm not sure if the lambda completes, it emitMetrics() will also exit, or complete.
There was a problem hiding this comment.
Hmm, I think if you declare it to be async, then yes, you need to await it to avoid the scenario you mention, where the lambda exits before the metrics have been emitted. But if it's just a straight synchronous function, I believe that would be the same as just writing the same code in-line in the main lambda function - it won't exit until the metrics have been written
| (statusesMapping.get(letterCommand.status) || 0) + 1, | ||
| ); | ||
| } | ||
| return letterCommand; |
There was a problem hiding this comment.
Would it be better for post-letters to build the statusMapping as a separate step, rather than complicating the mapToUpdateCommands function?
There was a problem hiding this comment.
I also don't feel good about adding extra logic to this function.
I was between populating the map in this function or letter-operations.enqueueLetterUpdateRequests(), because these functions already loop through the letter requests and I could populate the map at the same time.
I could iterate the requests and populate my map in post-letters directly, but that would slow down processing, and I was told by Mark that we already have performance issues.
I have kept the map as an optional parameter, but if we decide as a team I could always remove the logic from here and add it to post-letters
There was a problem hiding this comment.
I'd be surprised if populating the map separately turned out to have a significant impact on performance - ultimately it's just manipulating a small in-memory array
| PublishBatchRequestEntries: batch.map((element, index) => { | ||
| eventTypeCount.set( | ||
| element.type, | ||
| (eventTypeCount.get(element.type) || 0) + 1, |
There was a problem hiding this comment.
I'm wondering if it would be clearer to move the calculation of eventTypeCount outside of the batch loop - it might be simpler to calculate it in one go rather than a batch at a time
There was a problem hiding this comment.
I could add it a bit further up in the mapLetterToCloudEvent() function, but again I'm complicating the single-responsibility of the function.
I could also separately loop over the events one by one, but that would slow down the lambda.
There was a problem hiding this comment.
I wouldn't assume that doing it in a separate loop would slow down the lambda - if you're concerned, you could try both options and see if it makes a difference to the lambda timings
| import mapLetterToCloudEvent from "./mappers/letter-mapper"; | ||
| import { Deps } from "./deps"; | ||
| import { LetterForEventPub, LetterSchemaForEventPub } from "./types"; | ||
| import { string } from "zod"; |
There was a problem hiding this comment.
Does this need to be the zod string, or could it be the built-in type?
| const eventTypeCount = new Map<string, number>(); | ||
| const eventTypeCount: Map<string, number> = | ||
| populateEventTypeMap(cloudEvents); | ||
| // const eventTypeCount = new Map<string, number>(); |
There was a problem hiding this comment.
This commented out line can probably be removed
2 participants
Description
Context
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.