Skip to content

Commit a5ebe74

Browse files
narehayrapetyannarehayrapetyan
committed
Addressing comments
1 parent 67ff80f commit a5ebe74

File tree

5 files changed

+31
-37
lines changed

5 files changed

+31
-37
lines changed

‎mixer/adapter/cloudwatch/client.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ import (
1919
"github.com/aws/aws-sdk-go/service/cloudwatch"
2020
)
2121

22-
// NewCloudWatchClient creates a cloudwatch client
23-
func NewCloudWatchClient() *cloudwatch.CloudWatch {
22+
// newCloudWatchClient creates a cloudwatch client
23+
func newCloudWatchClient() *cloudwatch.CloudWatch {
2424
return cloudwatch.New(session.Must(session.NewSessionWithOptions(session.Options{
2525
SharedConfigState: session.SharedConfigEnable,
2626
})))

‎mixer/adapter/cloudwatch/cloudwatch.go‎

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ type (
5353
metricTypes map[string]*metric.Type
5454
}
5555

56-
// Handler holds data for the cloudwatch adapter handler
57-
Handler struct {
56+
// handler holds data for the cloudwatch adapter handler
57+
handler struct {
5858
metricTypes map[string]*metric.Type
5959
env adapter.Env
6060
cfg *config.Params
@@ -64,19 +64,19 @@ type (
6464

6565
// ensure types implement the requisite interfaces
6666
var _ metric.HandlerBuilder = &builder{}
67-
var _ metric.Handler = &Handler{}
67+
var _ metric.Handler = &handler{}
6868

6969
///////////////// Configuration-time Methods ///////////////
7070

71-
// NewHandler initializes a cloudwatch handler
72-
func NewHandler(metricTypes map[string]*metric.Type, env adapter.Env, cfg *config.Params, cloudwatch cloudwatchiface.CloudWatchAPI) adapter.Handler {
73-
return &Handler{metricTypes: metricTypes, env: env, cfg: cfg, cloudwatch: cloudwatch}
71+
// newHandler initializes a cloudwatch handler
72+
func newHandler(metricTypes map[string]*metric.Type, env adapter.Env, cfg *config.Params, cloudwatch cloudwatchiface.CloudWatchAPI) adapter.Handler {
73+
return &handler{metricTypes: metricTypes, env: env, cfg: cfg, cloudwatch: cloudwatch}
7474
}
7575

7676
// adapter.HandlerBuilder#Build
7777
func (b *builder) Build(ctx context.Context, env adapter.Env) (adapter.Handler, error) {
78-
cloudwatch := NewCloudWatchClient()
79-
return NewHandler(b.metricTypes, env, b.adpCfg, cloudwatch), nil
78+
cloudwatch := newCloudWatchClient()
79+
return newHandler(b.metricTypes, env, b.adpCfg, cloudwatch), nil
8080
}
8181

8282
// adapter.HandlerBuilder#SetAdapterConfig
@@ -96,22 +96,19 @@ func (b *builder) Validate() (ce *adapter.ConfigErrors) {
9696

9797
for k, v := range b.metricTypes {
9898
// validate handler config contains required metric config
99-
_, ok := b.adpCfg.GetMetricInfo()[k]
100-
if !ok {
99+
if _, ok := b.adpCfg.GetMetricInfo()[k]; !ok {
101100
ce = ce.Append("metricInfo", fmt.Errorf("metricInfo and instance config must contain the same metrics but missing %v", k))
102101
}
103102

104103
// validate that value type can be handled by the CloudWatch handler
105-
_, ok = supportedValueTypes[v.Value.String()]
106-
if !ok {
104+
if _, ok := supportedValueTypes[v.Value.String()]; !ok {
107105
ce = ce.Append("value type", fmt.Errorf("value of type %v is not supported", v.Value))
108106
}
109107

110108
// validate that if the value is a duration it has the correct cloudwatch unit
111109
if v.Value == istio_policy_v1beta1.DURATION {
112110
unit := b.adpCfg.GetMetricInfo()[k].GetUnit()
113-
_, ok = supportedDurationUnits[unit]
114-
if !ok {
111+
if _, ok := supportedDurationUnits[unit]; !ok {
115112
ce = ce.Append("duration metric unit", fmt.Errorf("value of type duration should have a unit of seconds, milliseconds or microseconds"))
116113
}
117114
}
@@ -130,14 +127,14 @@ func (b *builder) SetMetricTypes(types map[string]*metric.Type) {
130127
}
131128

132129
// HandleMetric sends metrics to cloudwatch
133-
func (h *Handler) HandleMetric(ctx context.Context, insts []*metric.Instance) error {
130+
func (h *handler) HandleMetric(ctx context.Context, insts []*metric.Instance) error {
134131
metricData := h.generateMetricData(insts)
135132
_, err := h.sendMetricsToCloudWatch(metricData)
136133
return err
137134
}
138135

139136
// Close implements client closing functionality if necessary
140-
func (h *Handler) Close() error {
137+
func (h *handler) Close() error {
141138
return nil
142139
}
143140

‎mixer/adapter/cloudwatch/config/config.proto‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,7 @@ syntax = "proto3";
2121
// The CloudWatch adapter enables Istio to deliver metrics to
2222
// [Amazon CloudWatch](https://aws.amazon.com/cloudwatch/).
2323
//
24-
// To push metrics to CloudWatch using this adapter you need create an IAM user
25-
// that has permissions to call cloudwatch APIs. The credentials for the user
26-
// need to be available on the instance the adapter is running on
24+
// To push metrics to CloudWatch using this adapter you must provide AWS credentials the AWS SDK.
2725
// (see [AWS docs](https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/setup-credentials.html)).
2826
//
2927
// To activate the CloudWatch adapter, operators need to provide configuration for the

‎mixer/adapter/cloudwatch/handler.go‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ const (
3434
metricDatumLimit = 20
3535
)
3636

37-
func (h *Handler) sendMetricsToCloudWatch(metricData []*cloudwatch.MetricDatum) (int, error) {
37+
func (h *handler) sendMetricsToCloudWatch(metricData []*cloudwatch.MetricDatum) (int, error) {
3838
cloudWatchCallCount := 0
3939
var multiError *multierror.Error
4040

@@ -44,8 +44,7 @@ func (h *Handler) sendMetricsToCloudWatch(metricData []*cloudwatch.MetricDatum)
4444
size = len(metricData)
4545
}
4646

47-
err := h.putMetricData(metricData[i:size])
48-
if err == nil {
47+
if err := h.putMetricData(metricData[i:size]); err == nil {
4948
cloudWatchCallCount++
5049
} else {
5150
multiError = multierror.Append(multiError, err)
@@ -55,7 +54,7 @@ func (h *Handler) sendMetricsToCloudWatch(metricData []*cloudwatch.MetricDatum)
5554
return cloudWatchCallCount, multiError.ErrorOrNil()
5655
}
5756

58-
func (h *Handler) generateMetricData(insts []*metric.Instance) []*cloudwatch.MetricDatum {
57+
func (h *handler) generateMetricData(insts []*metric.Instance) []*cloudwatch.MetricDatum {
5958
metricData := make([]*cloudwatch.MetricDatum, 0, len(insts))
6059

6160
for _, inst := range insts {
@@ -68,6 +67,7 @@ func (h *Handler) generateMetricData(insts []*metric.Instance) []*cloudwatch.Met
6867

6968
value, err := getNumericValue(inst.Value, cwMetric.GetUnit())
7069
if err != nil {
70+
// do not fail putting all instances into cloudwatch if one does not have a parsable value
7171
_ = h.env.Logger().Errorf("could not parse value %v", err)
7272
continue
7373
}
@@ -128,14 +128,13 @@ func getDurationNumericValue(v time.Duration, unit config.Params_MetricDatum_Uni
128128
}
129129
}
130130

131-
func (h *Handler) putMetricData(metricData []*cloudwatch.MetricDatum) error {
131+
func (h *handler) putMetricData(metricData []*cloudwatch.MetricDatum) error {
132132
input := cloudwatch.PutMetricDataInput{
133133
Namespace: aws.String(h.cfg.Namespace),
134134
MetricData: metricData,
135135
}
136136

137-
_, err := h.cloudwatch.PutMetricData(&input)
138-
if err != nil {
137+
if _, err := h.cloudwatch.PutMetricData(&input); err != nil {
139138
return h.env.Logger().Errorf("could not put metric data into cloudwatch: %v. %v", input, err)
140139
}
141140

‎mixer/adapter/cloudwatch/handler_test.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -65,19 +65,19 @@ func TestPutMetricData(t *testing.T) {
6565
{MetricName: aws.String("testMetric"), Value: aws.Float64(1)},
6666
},
6767
"put metric data",
68-
NewHandler(nil, env, cfg, &mockFailCloudWatchClient{}),
68+
newHandler(nil, env, cfg, &mockFailCloudWatchClient{}),
6969
},
7070
{
7171
[]*cloudwatch.MetricDatum{
7272
{MetricName: aws.String("testMetric"), Value: aws.Float64(1)},
7373
},
7474
"",
75-
NewHandler(nil, env, cfg, &mockCloudWatchClient{}),
75+
newHandler(nil, env, cfg, &mockCloudWatchClient{}),
7676
},
7777
}
7878

7979
for _, c := range cases {
80-
h, ok := c.handler.(*Handler)
80+
h, ok := c.handler.(*handler)
8181
if !ok {
8282
t.Error("Test case has the wrong type of handler.")
8383
}
@@ -217,7 +217,7 @@ func TestSendMetricsToCloudWatch(t *testing.T) {
217217
Namespace: "istio-mixer-cloudwatch",
218218
}
219219

220-
h := NewHandler(nil, env, cfg, &mockCloudWatchClient{})
220+
h := newHandler(nil, env, cfg, &mockCloudWatchClient{})
221221

222222
cases := []struct {
223223
metricData []*cloudwatch.MetricDatum
@@ -229,7 +229,7 @@ func TestSendMetricsToCloudWatch(t *testing.T) {
229229
}
230230

231231
for _, c := range cases {
232-
h, ok := h.(*Handler)
232+
h, ok := h.(*handler)
233233
if !ok {
234234
t.Error("Test case has the wrong type of handler.")
235235
}
@@ -266,13 +266,13 @@ func TestGenerateMetricData(t *testing.T) {
266266
}{
267267
// empty instances
268268
{
269-
NewHandler(nil, env,
269+
newHandler(nil, env,
270270
generateCfgWithUnit(config.Count),
271271
&mockCloudWatchClient{}),
272272
[]*metric.Instance{}, []*cloudwatch.MetricDatum{}},
273273
// timestamp value
274274
{
275-
NewHandler(nil, env,
275+
newHandler(nil, env,
276276
generateCfgWithNameAndUnit("requestduration", config.Milliseconds),
277277
&mockCloudWatchClient{}),
278278
[]*metric.Instance{
@@ -292,7 +292,7 @@ func TestGenerateMetricData(t *testing.T) {
292292
},
293293
// count value and dimensions
294294
{
295-
NewHandler(nil, env,
295+
newHandler(nil, env,
296296
generateCfgWithNameAndUnit("requestcount", config.Count),
297297
&mockCloudWatchClient{}),
298298
[]*metric.Instance{
@@ -321,7 +321,7 @@ func TestGenerateMetricData(t *testing.T) {
321321
}
322322

323323
for _, c := range cases {
324-
h, ok := c.handler.(*Handler)
324+
h, ok := c.handler.(*handler)
325325
if !ok {
326326
t.Error("Test case has the wrong type of handler.")
327327
}

0 commit comments

Comments
 (0)