Skip to content

Support send_profile_events settings to reduce network flow#89588

Merged
alexey-milovidov merged 8 commits intoClickHouse:masterfrom
kavirajk:kavirajk/support-send-profile-events-settings
Nov 9, 2025
Merged

Support send_profile_events settings to reduce network flow#89588
alexey-milovidov merged 8 commits intoClickHouse:masterfrom
kavirajk:kavirajk/support-send-profile-events-settings

Conversation

@kavirajk
Copy link
Copy Markdown
Contributor

@kavirajk kavirajk commented Nov 5, 2025 •
edited by clickhouse-gh bot
Loading

Changelog category (leave one):

  • New Feature

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

Added send_profile_events setting which allows clients to reduce network traffic when profile events are unused.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Details

Built on top of #80761 preserving original author's (@SpencerTorres) commits.

(NOTE: I was unable edit the original PR because I don't have permission on author's fork and not a maintainer to edit the PR as is. Hence the new PR)

For users who are doing many small async inserts over Native, it's possible that more data will be received from the server than sent by the client. The ProfileEvents packet was identified to be a large portion of this. To help reduce traffic, I have added a boolean setting called send_profile_events that defaults to 1. Clients can choose to disable profile events to reduce traffic. In the Go client for example, this data isn't used. It's parsed and immediately ignored. This can be applied per-query.

With profile events enabled

$ clickhouse-client --query "SELECT 1 SETTINGS send_profile_events=1" --print-profile-events 2>&1 | head -n 10
1
[clickbox] 2025.11.05 17:29:58 [ 0 ] Query: 1 (increment)
[clickbox] 2025.11.05 17:29:58 [ 0 ] SelectQuery: 1 (increment)
[clickbox] 2025.11.05 17:29:58 [ 0 ] InitialQuery: 1 (increment)
[clickbox] 2025.11.05 17:29:58 [ 0 ] QueriesWithSubqueries: 1 (increment)
[clickbox] 2025.11.05 17:29:58 [ 0 ] SelectQueriesWithSubqueries: 1 (increment)
[clickbox] 2025.11.05 17:29:58 [ 0 ] NetworkSendElapsedMicroseconds: 87 (increment)
[clickbox] 2025.11.05 17:29:58 [ 0 ] NetworkSendBytes: 67 (increment)
[clickbox] 2025.11.05 17:29:58 [ 0 ] GlobalThreadPoolJobs: 1 (increment)
[clickbox] 2025.11.05 17:29:58 [ 0 ] QueryPlanOptimizeMicroseconds: 34 (increment)

With profile events disabled

$ clickhouse-client --query "SELECT 1 SETTINGS send_profile_events=0" --print-profile-events 2>&1 | head -n 10
1

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Nov 5, 2025 •
edited
Loading

Workflow [PR], commit [fc60f8d]

Summary: ❌

job_name test_name status info comment
Upgrade check (amd_tsan) failure
Killed by signal (in clickhouse-server.log) FAIL cidb
Fatal message in clickhouse-server.log (see fatal_messages.txt) FAIL cidb
Killed by signal (output files) FAIL cidb
Found signal in gdb.log FAIL cidb
Upgrade check (amd_msan) failure
Killed by signal (in clickhouse-server.log) FAIL cidb
Fatal message in clickhouse-server.log (see fatal_messages.txt) FAIL cidb
Killed by signal (output files) FAIL cidb
Found signal in gdb.log FAIL cidb

@kavirajk kavirajk changed the title Kavirajk/support send profile events settings support send_profile_events settings to reduce network flow Nov 5, 2025
@kavirajk kavirajk changed the title support send_profile_events settings to reduce network flow Support send_profile_events settings to reduce network flow Nov 5, 2025
@clickhouse-gh clickhouse-gh bot added the pr-feature Pull request with new product feature label Nov 5, 2025
alexey-milovidov
alexey-milovidov approved these changes Nov 5, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

LGTM

@alexey-milovidov alexey-milovidov self-assigned this Nov 5, 2025
@alexey-milovidov alexey-milovidov mentioned this pull request Nov 5, 2025
Closed
1 task
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Nov 5, 2025

@kavirajk at least in some invocations, this particular profile event was not sent in the default mode.

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Nov 6, 2025 •
edited
Loading

Still a problem. Maybe use another profile event, e.g., Query or InitialQuery?

@kavirajk
Copy link
Copy Markdown
Contributor Author

kavirajk commented Nov 6, 2025

Still the flaky check is failing for this test even with Query profile event. The exact command from the test log passes in my local under default CH server settings.

Is there any way I can reproduce this failure in local? Tried this from the failure log python -m ci.praktika run "Stateless tests (amd_asan, flaky check)" --test TEST_NAME_1..TEST_NAME_N but that requires lots of additional file paths and tools to be installed.

azat
azat reviewed Nov 6, 2025
View reviewed changes
@azat
Copy link
Copy Markdown
Member

azat commented Nov 6, 2025

Is there any way I can reproduce this failure in local?

I was able to reproduce locally with ../tests/clickhouse-test --test-runs 100 -j 32 03418_send_profile_events_setting

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Nov 6, 2025

To clarify the behavior:

Flaky check uses the Thread Fuzzer, which randomly slows down threads to increase the probability of race conditions.
Flaky check uses the ASan build.

@kavirajk
Copy link
Copy Markdown
Contributor Author

kavirajk commented Nov 6, 2025 •
edited
Loading

Thanks @azat @alexey-milovidov for the help and explanation about Thread Fuzzer 👍

@alexey-milovidov what do you think about this comment from @azat
#80761 (comment)

His concern about "the settings to change the protocol (no ProfileEvents packets) is odd"

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Nov 6, 2025

His concern about "the settings to change the protocol (no ProfileEvents packets) is odd"

We don't change the structure of the protocol, so it's not a big problem. However, there are other concerns.
Commented there #80761 (comment)

@azat
Copy link
Copy Markdown
Member

azat commented Nov 7, 2025

I've tested clickhouse_driver and it can be broken with send_profile_events=0 for INSERT - https://gist.github.com/azat/78ab100c053bd7e01fd0e97c7e5b003a

@kavirajk
Copy link
Copy Markdown
Contributor Author

kavirajk commented Nov 7, 2025 •
edited
Loading

I've tested clickhouse_driver and it can be broken with send_profile_events=0 for INSERT -

Thanks for testing @azat. I think setting to 0 explicitly won't be huge problem(as long the it's not broken by default, without that setting in the query), because probably user knows what he or she is doing? and know probably know why it's broken (because server not sending this packets)?. I do agree that clients need to handle this in newer version to avoid breaking like this (and document it of course).

Edit: Also thinking bit more about it. Isn't this the same problem for any new settings we add to ClickHouse? (say older clients -> talking to newer ClickHouse server). And still client can pass these newer settings in query that may have different behavior from the server side.

@kavirajk
Copy link
Copy Markdown
Contributor Author

kavirajk commented Nov 7, 2025

@azat thinking bit more about our discussion today. If we design this change so that server and client negotiate client revision during hello messages of the protocol (so that server can decide whether to send ProfileEvents packets or not to the client, without breaking).

The problem is then server has to maintain all these client revisions to make that decision? I can imagine it can easily get out of hands considering there can be n number of clients out there (both official/non-official, internal, external, etc).

Looks to me more complex. May be I miss something?

@kavirajk
Copy link
Copy Markdown
Contributor Author

kavirajk commented Nov 7, 2025

@alexey-milovidov what do you think about the above discussions? Curious to know your thoughts.

(btw. The failing tests on this PR seems unrelated, looks CH server got killed for some reason)

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Nov 9, 2025

The change is reasonable. Having this option makes sense even if it can be misused. Let's merge.

@alexey-milovidov alexey-milovidov merged commit d352ca1 into ClickHouse:master Nov 9, 2025
127 of 131 checks passed
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Nov 9, 2025
@alexey-milovidov alexey-milovidov mentioned this pull request Nov 10, 2025
Merged
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Nov 10, 2025

@kavirajk, sorry, I just noticed that the change in SettingsChangesHistory was incorrect. Please resubmit.
The old value should be true. Otherwise, you'll break all cloud customers.

@kavirajk kavirajk mentioned this pull request Nov 10, 2025
Closed
@erezrokah
Copy link
Copy Markdown

erezrokah commented Nov 12, 2025

Is there a place I can track the re-submission? Is it about changing this line https://github.com/ClickHouse/ClickHouse/pull/89588/files#diff-f14e258a31a2fffb2137cf9aec96924ebd422ccd6f588bcfca181687ff8732faR44

to {"send_profile_events", false, false, "New setting. Whether to send profile events to the clients."},?

@kavirajk kavirajk mentioned this pull request Nov 13, 2025
Merged
1 task
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Nov 13, 2025

@erezrokah, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat left review comments

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

pr-feature Pull request with new product feature pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kavirajk @alexey-milovidov @azat @erezrokah @robot-ch-test-poll1 @SpencerTorres