Feature/ccm 17116 metrics#329
Conversation
af7676d to
75df6cf
Compare
| ContinuationToken=continuation_token | ||
| ) | ||
|
|
||
| def list_messages(self, max_results: Optional[int] = None, workflow_filter: Optional[str] = None) -> List[str]: |
There was a problem hiding this comment.
Signature taken from https://github.com/NHSDigital/mesh-client/blob/3.2.3/mesh_client/__init__.py#L512
| self.__log.info('Polling for messages') | ||
|
|
||
| # Record how many messages are in the mailbox | ||
| remaining_messages = self.__mesh_client.list_messages() |
There was a problem hiding this comment.
using list_messages method instead of iterate_all_messages because as per documentation the iterate is slower (see https://github.com/NHSDigital/mesh-client/blob/3.2.3/mesh_client/__init__.py#L839)
…r lambdas and in python CCM-17116: Updating dependency for the events publisher in the nodejs lambdas CCM-17116: replacing console.log as its wrapped by winston CCM-17116: Adding metric for total senders CCM-17116: Rename mesh metrics CCM-17116: Updated event_publisher to include the status when recording metrics CCM-17116: Updated metrics when recording events where the status can be different between events published
75df6cf to
c6dd8b3
Compare
| def record(self, value, **kwargs): | ||
|
|
||
| metric_name = kwargs.get('name', self.name) | ||
| """ | ||
| method for reporting metric | ||
| """ |
There was a problem hiding this comment.
The docstring should be on the first line after the function declaration
| def record(self, value, **kwargs): | |
| metric_name = kwargs.get('name', self.name) | |
| """ | |
| method for reporting metric | |
| """ | |
| def record(self, value, **kwargs): | |
| """ | |
| method for reporting metric | |
| """ | |
| metric_name = kwargs.get('name', self.name) |
| def record(self, value): | ||
| def record(self, value, **kwargs): | ||
|
|
||
| metric_name = kwargs.get('name', self.name) |
There was a problem hiding this comment.
I don't think the new "name override" logic has been covered in the unit tests.
| let key = event.type; | ||
| if (event.type === PRINT_LETTER_TRANSITIONED_EVENT_TYPE) { | ||
| const status = (event as Record<string, any>).data?.status as | ||
| | string | ||
| | undefined; | ||
| if (status) key = `${event.type}_${status}`; | ||
| } |
There was a problem hiding this comment.
This seems a bit smelly to me. Mainly the hardcoding of the event name. Not sure if we need/want to change it now, but an option would be to pass in a metricName function.
| let key = event.type; | |
| if (event.type === PRINT_LETTER_TRANSITIONED_EVENT_TYPE) { | |
| const status = (event as Record<string, any>).data?.status as | |
| | string | |
| | undefined; | |
| if (status) key = `${event.type}_${status}`; | |
| } | |
| const key = this.config.metricKeyResolver?.(event) ?? event.type; |
The updated method signature:
export interface EventPublisherDependencies {
eventBusArn: string;
dlqUrl: string;
logger: Logger;
sqsClient: SQSClient;
eventBridgeClient: EventBridgeClient;
metricHandler: MetricHandler;
metricKeyResolver?: (event: PublishableEvent) => string;
}/root/repos/nhs-notify-digital-letters/lambdas/print-status-handler/src/container.ts:
const eventPublisher = new EventPublisher({
eventBusArn: eventPublisherEventBusArn,
dlqUrl: eventPublisherDlqUrl,
logger,
sqsClient,
eventBridgeClient,
metricHandler: new MetricHandler(dlMetricsNamespace, [
{ Name: 'Environment', Value: environment },
]),
metricKeyResolver: (event) => {
const status = (event as Record<string, unknown>).data &&
((event as Record<string, unknown>).data as Record<string, unknown>).status as string | undefined;
return status ? `${event.type}_${status}` : event.type;
},
});876c129 to
d987b19
Compare
cf0360b to
907d690
Compare
968d635 to
2678f70
Compare
2678f70 to
d3a4379
Compare
Description
To summarise, the changes are:
Updated the event publisher in typescript and in python to record the events published as a metric (see Confluence for the list of metrics )
As the event publisher now publishes metrics, it needs the dimension environment, so adding in the configuration of all the lambdas the environment name
-- Lifted the metric-handler from comms-mgr
Consolidated existing metrics namespaces into the DL metrics namespace
left as an environment variable the DL_METRICS_NAMESPACE in case we want to have a different namespace, e.g. Digital Letters/MESH, Digital Letters/PDM
Kept as it was the existing metrics with configurable name.
See comment for the tests evidences.
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.