Conversation
…ds which aren't needed
mjewildnhs
force-pushed
the
feature/CCM-14200-event-driven-callback-delivery
branch
from
5dd5607 to
c8d84f7
Compare
mjewildnhs
force-pushed
the
feature/CCM-14200-event-driven-callback-delivery
branch
2 times, most recently
from
9447393 to
c90f89d
Compare
mjewildnhs
changed the base branch from
main
to
feature/CCM-14199-foundation
|
@copilot review |
|
@mjewildnhs I've opened a new pull request, #34, to work on those changes. Once the pull request is ready, I'll request review from you. |
mjewildnhs
changed the base branch from
feature/CCM-14199-foundation
to
main
lambdas/client-transform-filter-lambda/src/services/validators/event-validator.ts
Show resolved
Hide resolved
| /** | ||
| * Add persistent context that will be included in all subsequent log entries | ||
| */ | ||
| addContext(context: LogContext): void { |
There was a problem hiding this comment.
Are we going to include things like COMPONENT in log context like core does?
There was a problem hiding this comment.
I suggest we have the async calls to putMetric be an implementation detail, either by switching to the callback-based variant or just invoking the async method without await. Ideally we shouldn't need to wait for CloudWatch calls to succeed before continuing on with business logic.
It looks like we've partially gone down this road, but my suggestion would be all metric requests are submitted in the background without waiting for them to complete regardless of criticality.
| const correlationId = extractCorrelationId(rawEvent); | ||
| logger.addContext({ correlationId }); | ||
|
|
||
| logLifecycleEvent("received", { |
There was a problem hiding this comment.
Is it worth "received" and any other events being strongly typed? We could have it so the TypeScript typings apply to the full payload so we can't pass invalid keys for a given event.
|
|
||
| // Emit metric for event received | ||
| await metricsService.emitEventReceived( | ||
| eventType ?? "unknown", |
There was a problem hiding this comment.
Rather than having to ?? "unknown" everywhere, it might make sense for the metrics service to do this for us by allowing the property to be undefined and filling it in with the defaults if missing.
| await metricsService.emitDeliveryInitiated(clientId || "unknown"); | ||
|
|
||
| // Clear context for next event | ||
| logger.clearContext(); |
There was a problem hiding this comment.
Is there ever a chance of the handler being invoked multiple times in succession in the same NodeJS runtime? Considering we're using async / await everywhere, this could lead to the context being cleared before one request fully finishes, as different parts of the code will resume in a non-deterministic order.
We may instead want to have unique elements of the logging context always be passed around in log calls to avoid this situation.
| }); | ||
| await metricsService.emitTransformationFailure( | ||
| eventErrorType, | ||
| "TransformationError", |
There was a problem hiding this comment.
I think these also might benefit from strong typing
| functions: 100, | ||
| lines: 100, | ||
| statements: -10, | ||
| branches: 50, |
There was a problem hiding this comment.
Are these thresholds intentional for all work going forwards?
This reverts commit 4e15075.
mjewildnhs
force-pushed
the
feature/CCM-14200-event-driven-callback-delivery
branch
from
84de504 to
fd433af
Compare
mjewildnhs
force-pushed
the
feature/CCM-14200-event-driven-callback-delivery
branch
from
c412701 to
6287115
Compare
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.