ELI-578 consumer_id - campaign_config mapping #504
Conversation
Karthikeyannhs
force-pushed
the
ELI-578/consumer-campaign-config-mappings
branch
from
e1c80b4 to
60b4954
Compare
Karthikeyannhs
force-pushed
the
ELI-578/consumer-campaign-config-mappings
branch
from
7a99d82 to
72f6d0c
Compare
…-mappings' into ELI-578/consumer-campaign-config-mappings
There was a problem hiding this comment.
CONSUMER_MAPPING_BUCKET_NAME = var.eligibility_consumer_mappings_bucket_name is the change here. Rest are just lint (spacings)
| self.bucket_name = bucket_name | ||
|
|
||
| def get_permitted_campaign_ids(self, consumer_id: ConsumerId) -> list[CampaignID] | None: | ||
| objects = self.s3_client.list_objects(Bucket=self.bucket_name).get("Contents") |
There was a problem hiding this comment.
Couldn't we set the filename as a constant (e.g. consumer_mapping.json) and avoid this list object (e.g. just do the get?)
There was a problem hiding this comment.
We have followed the same pattern that has been used for campaign config.
I think, we will need to revisit this as part of a review on the management of consumer mapping and campaign config.
Things to consider; do we want all consumers mapped in one file, or easier to manage one file per consumer? etc.
There was a problem hiding this comment.
Agree this whole thing might need a rethink. We're likely adding another 50-100ms of latency with this MR, so still just about sits under the arbitrary 200ms.
'do we want all consumers mapped in one file, or easier to manage one file per consumer? etc.' - at the moment the approach only supports the former, so your choice is to stick with the list_objects latency and rework if we want multiple files (one per consumer) or go with the single file approach and then rework to support multiple files?
I think we'd want a fast following ticket to resolve this if we leave it as is
There was a problem hiding this comment.
(my temptation is read in one file into memory and deal with it - we can be smart about managing it e.g. have a mechanism to independently manage each consumer and generate a single file)
| self.s3_client = s3_client | ||
| self.bucket_name = bucket_name | ||
|
|
||
| def get_permitted_campaign_ids(self, consumer_id: ConsumerId) -> list[CampaignID] | None: |
There was a problem hiding this comment.
it's probably worth caching the consumer config, it's likely to be v. slow changing
There was a problem hiding this comment.
We can look into this along with the "nhs-product-id" implementation userstory.
Because there are few edgecases which we have to deal with.
There was a problem hiding this comment.
It is a good idea to look at caching for both consumer mapping, and campaign config.
I am not sure of the consequences of caching one, without the other.
Example, we cached consumer to campaign id, but campaign changes (id, maybe).
We also have to think about cache invalidation, as we don't want some warm lambda running with old config, and new lambdas running new config.
| self.s3_client = s3_client | ||
| self.bucket_name = bucket_name | ||
|
|
||
| def get_permitted_campaign_ids(self, consumer_id: ConsumerId) -> list[CampaignID] | None: |
There was a problem hiding this comment.
More generally, an alternative might be a new dynamoDB table - latency would hopefully be much lower as single key lookups are v speedy vs. s3
There was a problem hiding this comment.
But we might look at this in terms of revisiting where we store campaign config too
There was a problem hiding this comment.
I think this is an interesting alternative,
but I would suggest reading from dynamoDB be part of another story, that would maybe look at moving both consumer mapping and campaign config to dynamoDB (so we follow a consistent pattern for all read config).
| } | ||
|
|
||
| # Policy doc for S3 Consumer Mappings bucket | ||
| data "aws_iam_policy_document" "s3_consumer_mapping_bucket_policy" { |
There was a problem hiding this comment.
lambda_consumer_mapping_read_policy might be clearer? But probably needs a wider revisit to ensure all our terraform names are sensible / explain what the purpose it - a lot of my stuff from early on is likely confusing to me now....
| if permitted_campaign_ids: | ||
| permitted_campaign_configs: list[CampaignConfig] = [ | ||
| campaign for campaign in campaign_configs if campaign.id in permitted_campaign_ids | ||
| ] |
There was a problem hiding this comment.
do we need a log.warning if a configured permission id doesn't match any active campaigns?
3 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.