-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Description
Describe the bug
#34648 does not work as expected and would result in failed deployments or metrics that are not what the developer expected. dimensions and dimensionsMap are not compatible.
MetricFilter dimensions are selectors e.g. '$.foo' and the previous PR's 'Bar' and 'Baz' values would result in a failed deployment with the following error message:
"Invalid request provided: AWS::Logs::MetricFilter. Invalid metric transformation: dimension value must be a valid selector ..."
Metric dimensionsMap are metric dimension keys, and metric values that would otherwise be selected by the selector in dimensions.
example
e.g. a log line:
{ "foo": "baz" }with MetricFilter dimensions: { Foo: '$.foo' } would need to have a Metric dimensionsMap: { "Foo": "baz" } to be able to have an instance of the resulting metric to be represented correctly.
The original PR shows a misunderstanding of MetricFilter vs Metric, which is very common
Given this is the case, I do not understand how this integration test provided in #34648 would have passed and provided the resulting snapshot output. The stack in question will fail with the above mentioned error message due to the 'Bar' and 'Baz' values not being metric transformation selectors.
Regression Issue
- Select this option if this issue appears to be a regression.
Last Known Working CDK Library Version
2.200.2
Expected Behavior
I expected it to fail deployment using the feature in question due to what I believe to be a misunderstanding of Metric Filters vs Metrics in #34648
Current Behavior
- Failed deployment for the integration test
- For cases that do use selectors correctly e.g.
dimensions: { Foo: '$.foo' }that would result indimensionsMap: { Foo: '$.foo' }would result in aMetricinstance that will represent a metric that would never occur, given the value'$.foo'is a selector, not a value. The log line that would produce this metric would be{"foo":"$.foo"}, which is not what I believe the original author expects
Reproduction Steps
- attempt to create a stack with the constructs from fix(logs): exposed metric from the metric filter will now include the dimension map #34648 integ test or attempt to run that integ test with
--forcewhich will flag up that it will fail - or if using selectors correctly, call
.metric().createAlarm(...)on yourMetricFilterand then log a value to the log group that you think matches the dimensions provided e.g.{"foo":"baz"}. The metric for log line{"foo":"baz"}where dimension valueFoo: 'baz'will not trigger an alarm because the alarm is setup against a metric with value'$.foo'and would only trigger for log line{"foo":"$.foo"}
Possible Solution
revert #34648
Additional Information/Context
No response
AWS CDK Library version (aws-cdk-lib)
2.201.0
AWS CDK CLI version
2.1022.0
Node.js Version
22
OS
Ubuntu
Language
TypeScript
Language Version
5.6.3
Other information
Do the integ tests just pass fully and code can reach all the way to being released without needing to be deployed? i.e. are we just trusting these snapshots have not been created without the successful deployment first, or potentially interfered with after a different successful deployment? I have confirmed that this integ indeed fails for me when being forced to deploy rather than trust the matched snapshot.