[ClickHouse-57381]Implement MemoryCredits profile event to track memory usage over time. by Zawa-ll · Pull Request #66478 · ClickHouse/ClickHouse

Zawa-ll · 2024-07-13T18:21:35Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Implement MemoryCredits profile event to track memory usage over time.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Documentation Information:

Motivation: This feature is useful for distinguishing between queries with higher peak memory usage for a short period and queries that use less memory but for a much longer period.
Parameters:
- MemoryCredits: A profile event that counts the amount of microseconds*bytes (or megabytes) consumed by a query.
Example use:

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CI Settings (Only check the boxes if you know what you are doing):

Allow: All Required Checks
Allow: Stateless tests
Allow: Stateful tests
Allow: Integration Tests
Allow: Performance tests
Allow: All Builds
Allow: batch 1, 2 for multi-batch jobs
Allow: batch 3, 4, 5, 6 for multi-batch jobs

Exclude: Style check
Exclude: Fast test
Exclude: All with ASAN
Exclude: All with TSAN, MSAN, UBSAN, Coverage
Exclude: All with aarch64, release, debug

This addresses issue #57381.

alexey-milovidov · 2024-07-14T19:01:56Z

src/Common/ProfileEvents.h

No, it should be at the place of the usage.

alexey-milovidov · 2024-07-14T19:02:09Z

src/Common/ProfileEvents.cpp

Not clear enough what credits are.

alexey-milovidov · 2024-07-14T19:02:36Z

src/Common/MemoryTracker.h

Having these two members looks excessive.

Sure, these two variables were moved to the function MemoryTracker::updateMemoryCredits()

void MemoryTracker::updateMemoryCredits() { + static Stopwatch stopwatch; + static size_t previous_value = 0; size_t current_value = amount; if (current_value > previous_value && current_value - previous_value > threshold) { size_t delta = (current_value - previous_value) * stopwatch.elapsedMicroseconds(); ProfileEvents::increment(ProfileEvents::MemoryCredits, delta); previous_value = current_value; stopwatch.restart(); } }

If you have a better approach, please let me know! Thank you!

alexey-milovidov · 2024-07-14T19:02:44Z

src/Common/MemoryTracker.cpp

Missing newline.

alexey-milovidov · 2024-07-14T19:02:57Z

src/Common/MemoryTracker.cpp

It can be negative.

src/Common/AsynchronousMetrics.cpp

alexey-milovidov

Missing tests.

Zawa-ll · 2024-07-14T22:42:05Z

@alexey-milovidov Thank you for your response! Changes were made!

Also I notice that the workflow required approval... Can you approve this for me? Thank you!

robot-ch-test-poll4 · 2024-07-14T23:23:42Z

This is an automated comment for commit d76c6cf with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	❌ failure

Successful checks

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	✅ success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	✅ success

Algunenano · 2024-07-15T18:39:49Z

src/Common/MemoryTracker.h

Why mix 2 completely different variables? The original threshold was to throw logical errors in debug mode, but now you are using it to update memory credits too, so we can't change one without changing the other.

Changes were made! I misunderstand the relationship between the two variables.

UnamedRus · 2024-07-16T09:34:31Z

src/Common/ProfileEvents.cpp

Arent microseconds*bytes can be really big number (plus, with high randomity in it)?
May be, it make sense to have it as microseconds*MB or round lower bits, but keep bytes then.

No, let's keep bytes.

Zawa-ll · 2024-07-16T18:16:15Z

@alexey-milovidov Hi I just notice that the build failed for some reason... I don't think they are quite relevant to my code changes, can you provide me with some clue on how this is supposed to be fixed?

alexey-milovidov · 2024-07-17T02:02:10Z

Not only builds have failed, but basically everything in this pull request. Read the details of the corresponding checks...

Zawa-ll · 2024-07-19T01:30:25Z

Not only builds have failed, but basically everything in this pull request. Read the details of the corresponding checks...

Logic for handling data race has been added!

Zawa-ll · 2024-08-02T17:38:09Z

@alexey-milovidov Can you please take a look at this PR again? I double check the failing checks and still await for some instructions on how this failing check caused by ec2_admin should be handled...

INFO:botocore.credentials:Found credentials from IAM Role: ec2_admin
Check if rerun for name: [Stateless tests (msan)], extended name [Stateless tests (msan) [1/3]]
Commit status or Build Report is already present - job will be skipped with status: [failure]
{}
Error: Process completed with exit code

…proach

…r stable test results.

Remove logic for global threshold variables Remove logic for global threshold variable and create a local_variables

Zawa-ll · 2024-09-04T03:15:02Z

Updates: Based on @ Avogar, some falky tests still needs to be fixed,I will continue working on this one once they are merged, please do not close this PR, thank you!!!

clickhouse-gh · 2024-10-08T13:14:24Z

Dear @alexey-milovidov, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.