New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add detailed metrics to HttpApi #8510
Add detailed metrics to HttpApi #8510
Conversation
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.
|
4514954
to
6afb7ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BaptistG thanks for giving that a spin! That looks very promising. Please see my comments
@@ -130,7 +130,11 @@ class HttpApiEvents { | |||
ApiId: { Ref: this.provider.naming.getHttpApiLogicalId() }, | |||
StageName: '$default', | |||
AutoDeploy: true, | |||
DefaultRouteSettings: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create this configuration only if provider.httpApi.metrics
is provided (technically provider.httpApi.metrics != null
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make default a no setting and not false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will make the change based on your answer for #8510 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you @BaptistG!
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.