Template updates for Frequency Capping #784
Conversation
kookster
commented
Sep 22, 2024
•
kookster
commented
- Analytics Lambda
- Dovetail Router
| - dynamodb:DescribeTable | ||
| - dynamodb:DescribeTimeToLive | ||
| - dynamodb:GetItem | ||
| - dynamodb:Query |
There was a problem hiding this comment.
DTR should only need to read data, no write/update/delete actions
| - dynamodb:GetItem | ||
| - dynamodb:Query | ||
| Effect: Allow | ||
| Resource: !Split |
There was a problem hiding this comment.
This split/sub/join/split insanity was only needed for a short time when the arrangements table was being moved from a regional table to a global table, and we were accessing both. I don't believe that's the case any more.
So for this I would either simplify this back to a single !Ref, or probably FrequencyDynamodbTableName should to be renamed to something that indicates that it can/does contain multiple table names
There was a problem hiding this comment.
Ah, I see, I made it consistent with the other DDB tables, but I can simplify it as there is only one table
| Threshold: 0 | ||
| TreatMissingData: notBreaching | ||
|
|
||
| AnalyticsFrequencyFunctionInsertsMetricFilter: |
There was a problem hiding this comment.
The metrics generated by these next few metric filters don't seem to be used anywhere. Do we want to put them on a dashboard or alarm on them? Or are they just for reference purposes, should the need arise?
There was a problem hiding this comment.
Looking at the other lambdas in this template, I created equivalent metrics to what they had defined.
Is there someplace else I need to add these, some other dashboard template I don't remember?
| AlarmName: !Sub WARN [Dovetail-Analytics] Frequency Lambda function <${EnvironmentTypeAbbreviation}> INVOCATIONS ERRORS (${RootStackName}) | ||
| AlarmDescription: !Sub >- | ||
| ${EnvironmentType} Dovetail Analytics Frequency Lambda function is | ||
| failing, but tktktk. |
There was a problem hiding this comment.
heh, copied from other alarms in the same file with the same tktktk description
| - dynamodb:UpdateItem | ||
| Effect: Allow | ||
| # TODO: can this be done with an AWS::Partition Sub? | ||
| Resource: !Split |
There was a problem hiding this comment.
See other comment about this split/sub/join stuff
spire/templates/apps-300A.yml
Outdated
| @@ -152,6 +152,8 @@ Resources: | |||
| DovetailCountedKinesisStreamName: !Ref DovetailCountedKinesisStreamName | |||
| DovetailRouterHosts: !Sub /prx/${EnvironmentTypeAbbreviation}/dovetail-analytics/DOVETAIL_ROUTER_HOSTS | |||
| DovetailRouterApiTokens: !Sub /prx/${EnvironmentTypeAbbreviation}/dovetail-analytics/DOVETAIL_ROUTER_API_TOKENS | |||
| FrequencyDynamodbTableName: !Sub /prx/${EnvironmentTypeAbbreviation}/dovetail-analytics/FREQUENCY_DDB_TABLE | |||
There was a problem hiding this comment.
These Parameter Store parameters are using a naming convention that predates the current standard, and the few parameters that remain from back then are slated to be cleaned up.
It would be ideal if we didn't add any new parameters that don't follow the current standard, but if it makes more sense to deal with this later, let's make sure they at least get added to that issue.
There was a problem hiding this comment.
so, for this ddb, it will be a global table like the arrangements DDB table, so I don't believe it needs the optional region, I will add Spire in there
| - !Split [",", !Ref FrequencyDynamodbTableName] | ||
| - Action: sts:AssumeRole | ||
| Effect: Allow | ||
| Resource: !Split [",", !Ref FrequencyDynamodbAccessRoleArn] |
There was a problem hiding this comment.
Similar situation here: I believe we expect this to be a single role ARN now, so no need to Split.
| @@ -739,6 +741,25 @@ Resources: | |||
| Effect: Allow | |||
| Principal: | |||
| Service: ecs-tasks.amazonaws.com | |||
| - Action: | |||
There was a problem hiding this comment.
This statement needs to go under Policies, not the AssumePolicyDocument. See https://github.com/PRX/Infrastructure/blob/main/spire/templates/apps/feeder.yml#L599 for an example
