Skip to content
This repository was archived by the owner on Feb 12, 2022. It is now read-only.

Conversation

prat1kbhujbal
Copy link

Issue #, if available:

Description of changes:
I want to monitor data having these units on cloudwatchmetrics. So I have added some more commonly used units. Kindly merged it to master if it's correct.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@AustinDeric
Copy link

AustinDeric commented Mar 17, 2021

This is a great addition. We also need more units. Good to go as is, but it might be worth separating units out into a separate message because that could (and should) get larger. I would also like to see some naming conventions so that we can scale the units in an organized fashion.

@prat1kbhujbal
Copy link
Author

This is a great addition. We also need more units. Good to go as is, but it might be worth separating units out into a separate message because that could (and should) get larger. I would also like to see some naming conventions so that we can scale the units in an organized fashion.

Hi Austin, I have made a separate msg file for units as you suggested and added a few units and organised them. Please have a look at it and suggest any changes if require.
Also, will I be able to see these units on cloudwatch console through my custom metrics?

Copy link
Contributor

@dabonnie dabonnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, will I be able to see these units on cloudwatch console through my custom metrics?

I would recommend testing this feature and demonstrating the output as part of the review process, which would answer your last question :-).

@prat1kbhujbal
Copy link
Author

Also, will I be able to see these units on cloudwatch console through my custom metrics?

I would recommend testing this feature and demonstrating the output as part of the review process, which would answer your last question :-).

Hello @dabonnie, here's a screenshot of the error that I get when I launch the metric collector node with this custom message in use. And also I didn't get any data on cloudwatch console using this.
Screenshot from 2021-03-22 14-46-50 (1)
@mm318 @emersonknapp @jikawa-az

@emersonknapp
Copy link
Contributor

I'm sorry, I can't read screenshots - can you copy-paste any log output into a code block?

@prat1kbhujbal
Copy link
Author

prat1kbhujbal commented Mar 23, 2021

I'm sorry, I can't read screenshots - can you copy-paste any log output into a code block?

Hi @emersonknapp, this is the log output of error

[ WARN] [1616477742.231983343]: [EnumParseOverflowContainer] Encountered enum member watt which is not modeled in your clients. You should update your clients when you get a chance.
[ WARN] [1616477742.290441414]: [AWSErrorMarshaller] Encountered AWSError 'InvalidParameterValue': The parameter MetricData.member.7.Unit must be a value in the set [ Seconds, Microseconds, Milliseconds, Bytes, Kilobytes, Megabytes, Gigabytes, Terabytes, Bits, Kilobits, Megabits, Gigabits, Terabits, Percent, Count, Bytes/Second, Kilobytes/Second, Megabytes/Second, Gigabytes/Second, Terabytes/Second, Bits/Second, Kilobits/Second, Megabits/Second, Gigabits/Second, Terabits/Second, Count/Second, None ].
[ERROR] [1616477742.290639590]: [AWSClient] HTTP response code: 400
Exception name: InvalidParameterValue
Error message: The parameter MetricData.member.7.Unit must be a value in the set [ Seconds, Microseconds, Milliseconds, Bytes, Kilobytes, Megabytes, Gigabytes, Terabytes, Bits, Kilobits, Megabits, Gigabits, Terabits, Percent, Count, Bytes/Second, Kilobytes/Second, Megabytes/Second, Gigabytes/Second, Terabytes/Second, Bits/Second, Kilobits/Second, Megabits/Second, Gigabits/Second, Terabits/Second, Count/Second, None ].
5 response headers:
connection : close
content-length : 665
content-type : text/xml
date : Tue, 23 Mar 2021 05:35:41 GMT
x-amzn-requestid : de69125d-5adf-453c-8f16-5373f5471ccb
[ WARN] [1616477742.290797818]: [AWSClient] If the signature check failed. This could be because of a time skew. Attempting to adjust the signer.

@emersonknapp
Copy link
Contributor

The current set of metrics supported by this package are the ones laid out in the AWS CloudWatch documentation - https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_MetricDatum.html

The mapping between the MetricData and the AWS C++ SDK values (which themselves map to the strings from the documentation) happens in metric_object.h

The error message you're getting is from the following line, which cannot find a standard unit for the name "watt".

    datum.SetUnit(Aws::CloudWatch::Model::StandardUnitMapper::GetStandardUnitForName(unit_name));

Given this information, we have two choices:

  1. Do not introduce new unit names, considering they cannot be understood by AWS CloudWatch
  2. Introduce the new unit names, but explicitly map them to None for reporting to AWS CloudWatch. This allows the MetricData message to at least have this extra information, though it will not be represented on the cloud side.

@prat1kbhujbal
Copy link
Author

The current set of metrics supported by this package are the ones laid out in the AWS CloudWatch documentation - https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_MetricDatum.html

The mapping between the MetricData and the AWS C++ SDK values (which themselves map to the strings from the documentation) happens in metric_object.h

The error message you're getting is from the following line, which cannot find a standard unit for the name "watt".

    datum.SetUnit(Aws::CloudWatch::Model::StandardUnitMapper::GetStandardUnitForName(unit_name));

Given this information, we have two choices:

  1. Do not introduce new unit names, considering they cannot be understood by AWS CloudWatch
  2. Introduce the new unit names, but explicitly map them to None for reporting to AWS CloudWatch. This allows the MetricData message to at least have this extra information, though it will not be represented on the cloud side.

Thanks for the updates. I will look into it.

Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion - needs a decision on whether new types are worth adding here, given they cannot be represented in CloudWatch Metrics

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants