feat: Support CloudWatch as a metric provider #1338
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1338 +/- ##
==========================================
+ Coverage 81.35% 81.38% +0.03%
==========================================
Files 108 109 +1
Lines 14505 14685 +180
==========================================
+ Hits 11801 11952 +151
- Misses 2089 2111 +22
- Partials 615 622 +7
Continue to review full report at Codecov.
|
ryota-sakamoto
force-pushed
the
support-cloud-watch-metrics
branch
4 times, most recently
from
b9cd1ec to
24dcab9
Compare
ryota-sakamoto
force-pushed
the
support-cloud-watch-metrics
branch
4 times, most recently
from
ecc5061 to
77d06b7
Compare
Signed-off-by: Ryota Sakamoto <[email protected]>
Signed-off-by: Ryota Sakamoto <[email protected]>
Signed-off-by: Ryota Sakamoto <[email protected]>
Signed-off-by: Ryota Sakamoto <[email protected]>
Signed-off-by: Ryota Sakamoto <[email protected]>
ryota-sakamoto
force-pushed
the
support-cloud-watch-metrics
branch
from
77d06b7 to
a57f475
Compare
|
I have no idea why the codecov report is weird. |
jessesuen
requested changes
Aug 3, 2021
Signed-off-by: Ryota Sakamoto <[email protected]>
|
@ryota-sakamoto we're seeing if this feature could make the v1.1 release in next few weeks. Did you have thoughts about my proposed changes to the spec? |
|
@jessesuen |
Signed-off-by: Ryota Sakamoto <[email protected]>
jessesuen
reviewed
Aug 30, 2021
Comment on lines
205
to
231
| type CloudWatchMetricDataQuery struct { | ||
| Id *string `json:"Id,omitempty" protobuf:"bytes,1,opt,name=Id"` | ||
| Expression *string `json:"Expression,omitempty" protobuf:"bytes,2,opt,name=Expression"` | ||
| Label *string `json:"Label,omitempty" protobuf:"bytes,3,opt,name=Label"` | ||
| MetricStat *CloudWatchMetricStat `json:"MetricStat,omitempty" protobuf:"bytes,4,opt,name=MetricStat"` | ||
| Period *int32 `json:"Period,omitempty" protobuf:"varint,5,opt,name=Period"` | ||
| ReturnData *bool `json:"ReturnData,omitempty" protobuf:"bytes,6,opt,name=ReturnData"` | ||
| } | ||
|
|
||
| type CloudWatchMetricStat struct { | ||
| Metric *CloudWatchMetricStatMetric `json:"Metric,omitempty" protobuf:"bytes,1,opt,name=Metric"` | ||
| Period *int32 `json:"Period,omitempty" protobuf:"varint,2,opt,name=Period"` | ||
| Stat *string `json:"Stat,omitempty" protobuf:"bytes,3,opt,name=Stat"` | ||
| Unit string `json:"Unit,omitempty" protobuf:"bytes,4,opt,name=Unit"` | ||
| } | ||
|
|
||
| type CloudWatchMetricStatMetric struct { | ||
| Dimensions []CloudWatchMetricStatMetricDimension `json:"Dimensions,omitempty" protobuf:"bytes,1,opt,name=Dimensions"` | ||
| MetricName *string `json:"MetricName,omitempty" protobuf:"bytes,2,opt,name=MetricName"` | ||
| Namespace *string `json:"Namespace,omitempty" protobuf:"bytes,3,opt,name=Namespace"` | ||
| } | ||
|
|
||
| type CloudWatchMetricStatMetricDimension struct { | ||
| Name *string `json:"Name,omitempty" protobuf:"bytes,1,opt,name=Name"` | ||
| Value *string `json:"Value,omitempty" protobuf:"bytes,2,opt,name=Value"` | ||
| } | ||
|
|
There was a problem hiding this comment.
I like this new approach you use to use structured objects, instead of my suggestion of making CloudWatchMetricDataQuery being a string field. However, I think we'll need to modify this some more. I believe issues 1 and 2 may be because you copied exact datastructures from AWS go client and used as our own types:
- There are now a lot of new "API rule violations" because of the casing of the json field name. Kubernetes APIs require names to be camel-cased always starting with lowercase letter, but here we use uppercase in the field names. e.g.:
Name *string `json:"Name,omitempty" protobuf:"bytes,1,opt,name=Name"`- Some fields were made as pointers, when they should always have a value required. For example, I don't think these should be allowed to be pointers. Can we make everything non-pointers unless we need to explicitly distinguish between not supplied vs. empty?
type CloudWatchMetricStatMetricDimension struct {
Name *string `json:"Name,omitempty" protobuf:"bytes,1,opt,name=Name"`
Value *string `json:"Value,omitempty" protobuf:"bytes,2,opt,name=Value"`
}- With this approach for using structured data (instead of a giant string field), we will not allow some things to be parameterized with Analysis arguments. For example, the
Periodfield cannot be parameterized because it is anint32.
Period *int32 `json:"Period,omitempty" protobuf:"varint,5,opt,name=Period"`
My question is: is Period something we should allow to be parameterized? If so, i think this should be made into an intstrutil.IntOrString instead of an int32
There was a problem hiding this comment.
- I've fixed
- I've fixed the pointer which isn't necessary I refer https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_MetricDataQuery.html
- I've fixed for that I use
intstrutil.IntOrStringinstead ofint32
ryota-sakamoto
force-pushed
the
support-cloud-watch-metrics
branch
3 times, most recently
from
26fb8a2 to
5b2994e
Compare
ryota-sakamoto
force-pushed
the
support-cloud-watch-metrics
branch
from
5b2994e to
ccbbe43
Compare
ryota-sakamoto
force-pushed
the
support-cloud-watch-metrics
branch
8 times, most recently
from
fa224b9 to
79fa536
Compare
Signed-off-by: Ryota Sakamoto <[email protected]>
ryota-sakamoto
force-pushed
the
support-cloud-watch-metrics
branch
from
79fa536 to
2906197
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@jessesuen can we merge this PR? seems @ryota-sakamoto addressed review comments |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Ryota Sakamoto [email protected]
close #1014
Checklist:
"fix(controller): Updates such and such. Fixes #1234".