Add detailed metrics to HttpApi #8510
Conversation
BaptistG
force-pushed
the
feat/httpApi_customMetrics
branch
2 times, most recently
from
61f8452
to
4514954
Compare
Codecov Report
@@ Coverage Diff @@
## master #8510 +/- ##
=======================================
Coverage 87.26% 87.26%
=======================================
Files 255 255
Lines 9503 9503
=======================================
Hits 8293 8293
Misses 1210 1210
Continue to review full report at Codecov.
|
BaptistG
force-pushed
the
feat/httpApi_customMetrics
branch
from
4514954
to
6afb7ef
Compare
| @@ -130,7 +130,11 @@ class HttpApiEvents { | |||
| ApiId: { Ref: this.provider.naming.getHttpApiLogicalId() }, | |||
| StageName: '$default', | |||
| AutoDeploy: true, | |||
| DefaultRouteSettings: { | |||
There was a problem hiding this comment.
Let's create this configuration only if provider.httpApi.metrics is provided (technically provider.httpApi.metrics != null)
There was a problem hiding this comment.
Hey @medikoo , that's what I was initially going for but when testing I noticed that if you've set DefaultRouteSettings.DetailedMetricsEnabled to true, the only way to revert the change is to explicitely set it to false. If you just remove the DefaultRouteSettings in the CF template it will not deactivate detailed metrics.
(1) This activates metrics
"HttpApiStage": {
"Type": "AWS::ApiGatewayV2::Stage",
"Properties": {
"ApiId": {
"Ref": "HttpApi"
},
"StageName": "$default",
"AutoDeploy": true,
"DefaultRouteSettings": {
"DetailedMetricsEnabled": true
}
}
(2) This doesnt deactivate metrics if you've deployed (1) before
"HttpApiStage": {
"Type": "AWS::ApiGatewayV2::Stage",
"Properties": {
"ApiId": {
"Ref": "HttpApi"
},
"StageName": "$default",
"AutoDeploy": true,
}
(3) This deactivates metrics if (1) was deployed previously, if it wasn't, it is equivalent to (2)
"HttpApiStage": {
"Type": "AWS::ApiGatewayV2::Stage",
"Properties": {
"ApiId": {
"Ref": "HttpApi"
},
"StageName": "$default",
"AutoDeploy": true,
"DefaultRouteSettings": {
"DetailedMetricsEnabled": true
}
}
So if I make it a no setting, users will have to use the console to deactivate detailed metrics or will have to explicitely set metrics to false which might be confusing. Let me know what you think
There was a problem hiding this comment.
@BaptistG thanks for clarification. Indeed in such case it makes big sense to always configure DefaultRouteSettings. Still let's comment in that reasoning (so it's not accidentally removed with some cleanup)
| this.config = { | ||
| routes, | ||
| id: userConfig.id, | ||
| detailedMetrics: userConfig.detailedMetrics || false, |
There was a problem hiding this comment.
Let's make default a no setting and not false
There was a problem hiding this comment.
Noted, will make the change based on your answer for #8510 (comment)
Closes: #8190
Hi @medikoo ,
This PR implements the change suggested in #8190 . I used the detailedMetrics flag instead of metrics in httpApi because it is closer to the flag used by Cloudformation (DetailedMetricsEnabled).
I had to change my commit because when testing my change I noticed that once you've set DetailedMetricsEnabled to true, removing the property from DefaultRouteSettings will not disable the detailed metrics, to do so you have to set DetailedMetricsEnabled to false.