-
Notifications
You must be signed in to change notification settings - Fork 34
Add exponential histograms to histograms integration test #558
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
Conversation
d559b35
to
167e846
Compare
"github.com/aws/amazon-cloudwatch-agent-test/util/common" | ||
) | ||
|
||
func TestOTLPMetrics(t *testing.T) { | ||
instanceID := awsservice.GetInstanceId() |
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.
Pulling instance ID from IMDS instead of hardcoding to a dummy value so that concurrent integration tests don't interfere with each other
@@ -34,7 +36,6 @@ func TestOTLPMetrics(t *testing.T) { | |||
expected []struct { | |||
stat types.Statistic | |||
value float64 | |||
check func(t *testing.T, expected, actual float64) |
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.
This was actually completely unused
testGroupResult = t.TestRunner.Validate() | ||
} | ||
if testGroupResult.GetStatus() != status.SUCCESSFUL { | ||
log.Printf("%v test group failed due to %v", testName, err) |
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.
this would often print .. test group failed due to <nil>
as err
comes from RunAgent()
call and the status comes from the Validate()
call. Decided to just rework RunAgent to return an error only.
@@ -34,8 +34,8 @@ | |||
"aggregationTemporality": 1, | |||
"dataPoints": [ | |||
{ | |||
"startTimeUnixNano": START_TIME, | |||
"timeUnixNano": START_TIME, | |||
"startTimeUnixNano": METRIC_TIME, |
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.
I hate this weird template sed
logic...it would be better to use some tool to generate these metrics programmatically like otelgen: https://github.com/krzko/otelgen
Not your fault though. I started this
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.
I agree. I had Amazon Q write up a metric generator using the OTEL SDK. It got everything working except cumulative or delta exponential histograms. I then spent a day trying to figure out how to add exponential histograms, but I couldn't figure it out and eventually gave up. You can actually see the generator I had in the commit history.
uses: actions/setup-go@v3 | ||
with: | ||
go-version: ~1.20.0 | ||
go-version: ~1.23.0 |
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.
why are we bumping go as part of this?
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.
I was trying to upgrade the whole package from go 1.20 to go 1.23, hit an issue (forget what it was now...), and then tried to revert back, but I missed this. I don't think it should hurt as 1.23 is backwards compatible with our 1.20 go.mod file.
test/metric/stat.go
Outdated
AVERAGE types.Statistic = "Average" | ||
SAMPLE_COUNT types.Statistic = "SampleCount" | ||
MINIMUM types.Statistic = "Minimum" | ||
MAXUMUM types.Statistic = "Maximum" | ||
SUM types.Statistic = "Sum" |
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.
I started to remove the usage of these consts as the types
package in the SDK already defines these, but it would have made this PR even larger. I'd rather to that in a separate PR.
812b8e4
to
2603f8e
Compare
Description of the issue
We are adding support to push exponential histograms with cloudwatch (PMD) as a destination. This PR adds integration tests for this functionality.
Note
See companion PR for new agent functionality: aws/amazon-cloudwatch-agent#1677
Description of changes
License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Ran integration test locally with updated agent, all histogram tests pass
Integration test run: https://github.com/aws/amazon-cloudwatch-agent/actions/runs/16371811763
Histogram test: https://github.com/aws/amazon-cloudwatch-agent/actions/runs/16371811763/job/46261959442
Starting new run here after fixing merge conflict: https://github.com/aws/amazon-cloudwatch-agent/actions/runs/17081105626
Example of test failures w/o reasons (current behavior):
Example of test failure w/ reasons: