Increase PMD batch & buffer sizes by movence · Pull Request #481 · aws/amazon-cloudwatch-agent

movence · 2022-06-08T18:49:52Z

Description of the issue

The limit on datum size with PutMetricData has been increased to 1000, and CWA should be aligned with that change in order to send more datums per API call.

Description of changes

Increase the default datum size to 1000.

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

Running the existing integration test

Requirements

Before commit the code, please do the following steps.

Run make fmt and make fmt-sh
Run make linter

khanhntd · 2022-06-08T19:04:15Z

Another thing to notice would be the unit test for CWAgent is failing with different OS and its does not relate to our usual flakiness test.

--- FAIL: TestWrite (3.01s)
    cloudwatch_test.go:368: 
        	Error Trace:	cloudwatch_test.go:368
        	Error:      	Not equal: 
        	            	expected: 2
        	            	actual  : 1
        	Test:       	TestWrite
        	Messages:   	Expected number of calls (2) does not match the actual number of calls (1).
    cloudwatch_test.go:368: 
        	Error Trace:	cloudwatch_test.go:368
        	Error:      	Should be true
        	Test:       	TestWrite
FAIL
coverage: 65.7% of statements

Therefore, we would need to deep dive before moving on.
One side note from me is the coverage percentage is rather small. Are there any missing tests we would need to cover?

SaxyPandaBear · 2022-06-08T19:06:48Z

Is the change already deployed on the backend for this? Is there a way for us to manually/automatically integration test just to be sure it works and there isn't any other bottleneck or limiter?

movence · 2022-06-09T03:07:02Z

Another thing to notice would be the unit test for CWAgent is failing with different OS and its does not relate to our usual flakiness test.

I don't believe it's tied to any specific OSes but a minor failed test case which should have been fixed.

coverage percentage is rather small

Are you suggesting the overall code coverage should be bumped up?

khanhntd · 2022-06-09T03:17:02Z

I don't believe it's tied to any specific OSes but a minor failed test case which should have been fixed.

Yes it does not tied to any specific OS. Good job on fixing the test 👍 . However, the GitHub blocks running unit-test workflow until resolve the conflict

Are you suggesting the overall code coverage should be bumped up?

Yes. However, it's a no blocker from me. If we confident the existing unit tests has covered enough daily use case then it's fine by me.

Another non-block from me would be adding integration test , same as @SaxyPandaBear idea. For the idea, Seth has done similar to this (https://github.com/aws/amazon-cloudwatch-agent/tree/master/integration/test/metrics_number_dimension)

SaxyPandaBear · 2022-06-09T03:22:03Z

What coverage are you referring to @khanhntd? The change is tiny. Are there specific test cases that you think are missing? Otherwise I don't see what you really think we can improve on test coverage for a one variable change. The only possible thing I could see is integration testing, like what was done for the max dimensions change, but that's not unit test coverage.

…h-size # Conflicts: # plugins/outputs/cloudwatch/cloudwatch_test.go

khanhntd · 2022-06-09T03:28:56Z

What coverage are you referring to @khanhntd? The change is tiny. Are there specific test cases that you think are missing? Otherwise I don't see what you really think we can improve on test coverage for a one variable change. The only possible thing I could see is integration testing, like what was done for the max dimensions change, but that's not unit test coverage.

Yes. That's why I mentioned its a non-blocker from me consider only the variables change and the test bumping coverage can be done in another PR 👍.

adam-mateen

approved

I see existing unit test - TestIsFull() tests batch.isFull().
Unit tests do not make actual calls using the actual CloudWatch backend though.

…h-size

codecov-commenter · 2022-06-10T02:29:12Z

Codecov Report

Merging #481 (2d1a2b3) into master (e7a20ef) will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
+ Coverage   56.87%   57.00%   +0.12%     
==========================================
  Files         363      373      +10     
  Lines       16937    17638     +701     
==========================================
+ Hits         9633    10054     +421     
- Misses       6753     6993     +240     
- Partials      551      591      +40

Impacted Files	Coverage Δ
plugins/outputs/cloudwatch/cloudwatch.go	`75.00% <ø> (+1.62%)`	⬆️
translator/cmdutil/userutil_darwin.go
plugins/inputs/logfile/tail/tail_windows.go	`100.00% <0.00%> (ø)`
...gins/inputs/win_perf_counters/win_perf_counters.go	`68.15% <0.00%> (ø)`
plugins/inputs/logfile/tmpfile_windows.go	`40.00% <0.00%> (ø)`
...windows_event_log/wineventlog/wineventlogrecord.go	`59.09% <0.00%> (ø)`
...puts/logfile/tail/file_deleting_checker_windows.go	`36.36% <0.00%> (ø)`
...s/inputs/windows_event_log/wineventlog/sys_call.go	`47.36% <0.00%> (ø)`
...gins/inputs/windows_event_log/wineventlog/utils.go	`69.60% <0.00%> (ø)`
plugins/inputs/win_perf_counters/pdh.go	`65.47% <0.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7a20ef...2d1a2b3. Read the comment docs.

SaxyPandaBear

Lgtm

holding on to this

…h-size

SaxyPandaBear · 2022-07-08T17:42:40Z

Is the backend ready for this or are we still waiting?

khanhntd

LGTM.

khanhntd · 2022-07-21T11:48:35Z

+	defaultMaxDatumsPerCall        = 1000   // PutMetricData only supports up to 1000 data metrics per call by default
 	defaultMaxValuesPerDatum       = 150    // By default only these number of values can be inserted into the value list
-	bottomLinePayloadSizeToPublish = 200000 // Leave 9600B for the last datum buffer. 200KB payload size, 5:1 compression ratio estimate.
+	bottomLinePayloadSizeToPublish = 500000 // Leave 2400B for the last datum buffer. 500KB payload size, 5:1 compression ratio estimate.


Should sync up with internal: bottomLinePayloadSizeToPublish = 999 000 (0.999 MB) and delete 5:1 compression ratio estimate

Can you confirm the API limit is actually 1 MiB, as in 1024 * 1024 Bytes?
If so then this limit could be raised to 1023 KiB (1023 * 1024 Bytes).
I don't think it is critical since a payload of 1,000 metric datums is unlikely to reach 1023 KiB.
May as well be as accurate as possible, right?

SaxyPandaBear · 2022-08-09T13:16:03Z

Done in #523

increase PMD batch size to 1000

7f75e72

movence requested review from SaxyPandaBear, ZhenyuTan-amz, adam-mateen and khanhntd June 8, 2022 18:49

movence requested a review from a team as a code owner June 8, 2022 18:49

movence removed the request for review from a team June 8, 2022 18:50

khanhntd reviewed Jun 8, 2022

View reviewed changes

Comment thread plugins/outputs/cloudwatch/cloudwatch.go

update unit test

4c7f181

Merge remote-tracking branch 'upstream/master' into increase-pmd-batc…

a1cbf20

…h-size # Conflicts: # plugins/outputs/cloudwatch/cloudwatch_test.go

adam-mateen previously approved these changes Jun 9, 2022

View reviewed changes

movence added 2 commits June 9, 2022 22:16

Merge remote-tracking branch 'upstream/master' into increase-pmd-batc…

8df0ab0

…h-size

reapply unit test update

e94d496

movence dismissed adam-mateen’s stale review via e94d496 June 10, 2022 02:20

khanhntd added this to the 1.247354.0 milestone Jun 10, 2022

SaxyPandaBear previously approved these changes Jun 11, 2022

View reviewed changes

khanhntd reviewed Jun 21, 2022

View reviewed changes

Comment thread plugins/outputs/cloudwatch/cloudwatch.go

movence added 2 commits July 5, 2022 09:30

Merge remote-tracking branch 'upstream/master' into increase-pmd-batc…

a21ca96

…h-size

increase PMD buffer size to 100k with gzip compression ratio accounted

2d1a2b3

movence changed the title ~~Increase PMD batch size to 1000 per API call~~ Increase PMD batch & buffer sizes Jul 6, 2022

khanhntd approved these changes Jul 12, 2022

View reviewed changes

khanhntd reviewed Jul 21, 2022

View reviewed changes

SaxyPandaBear closed this Aug 9, 2022

Conversation

movence commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the issue

Description of changes

License

Tests

Requirements

Uh oh!

Uh oh!

khanhntd commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SaxyPandaBear commented Jun 8, 2022

Uh oh!

movence commented Jun 9, 2022

Uh oh!

khanhntd commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SaxyPandaBear commented Jun 9, 2022

Uh oh!

khanhntd commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adam-mateen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SaxyPandaBear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SaxyPandaBear commented Jul 8, 2022

Uh oh!

khanhntd left a comment

Choose a reason for hiding this comment

Uh oh!

khanhntd Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

adam-mateen Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaxyPandaBear commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

movence commented Jun 8, 2022 •

edited

Loading

khanhntd commented Jun 8, 2022 •

edited

Loading

khanhntd commented Jun 9, 2022 •

edited

Loading

khanhntd commented Jun 9, 2022 •

edited

Loading

adam-mateen left a comment •

edited

Loading

codecov-commenter commented Jun 10, 2022 •

edited

Loading

adam-mateen Jul 21, 2022 •

edited

Loading