Skip to content

Improve the UX of SYSTEM INSTRUMENT ADD/REMOVE#93345

Merged
pamarcos merged 7 commits intoClickHouse:masterfrom
pamarcos:system-instrument-ux-improvements
Jan 5, 2026
Merged

Improve the UX of SYSTEM INSTRUMENT ADD/REMOVE#93345
pamarcos merged 7 commits intoClickHouse:masterfrom
pamarcos:system-instrument-ux-improvements

Conversation

@pamarcos
Copy link
Copy Markdown
Member

@pamarcos pamarcos commented Jan 2, 2026 •
edited
Loading

Follow-up on #89173 based on this and this comments:

Improve the UX of SYSTEM INSTRUMENT ADD/REMOVE:

  • Use String literals for function names. I'm not considering it a backwards incompatible change since it was released in 25.12 and I doubt anyone's using it yet
  • Patch all functions that match. This is the same behavior as debuggers
  • Allow using function_name in REMOVE. This is a nice to have thing, but previously could be done with a subquery
SYSTEM INSTRUMENT REMOVE (SELECT id FROM system.instrumentation WHERE function_name = 'executeQuery')

Changelog category (leave one):

  • Improvement

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

Improve the UX of SYSTEM INSTRUMENT ADD/REMOVE: use String literals for function names, patch all functions that match and allow using function_name in REMOVE

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@pamarcos pamarcos mentioned this pull request Jan 2, 2026
Merged
1 task
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jan 2, 2026 •
edited by pamarcos
Loading

Workflow [PR], commit [eeedd36]

Summary: ❌

job_name test_name status info comment
Integration tests (arm_binary, distributed plan, 2/4) failure
test_userspace_page_cache/test.py::test_size_adjustment FAIL cidb, issue ISSUE EXISTS
Integration tests (amd_tsan, 5/6) failure
test_backup_restore_on_cluster/test_disallow_concurrency.py::test_concurrent_backups_on_same_node FAIL cidb, issue ISSUE EXISTS
test_backup_restore_on_cluster/test_disallow_concurrency.py::test_concurrent_backups_on_different_nodes FAIL cidb IGNORED
test_backup_restore_on_cluster/test_disallow_concurrency.py::test_concurrent_restores_on_same_node FAIL cidb IGNORED
test_backup_restore_on_cluster/test_disallow_concurrency.py::test_concurrent_restores_on_different_node FAIL cidb, issue ISSUE EXISTS
Integration tests (amd_tsan, 6/6) failure
test_throttling/test.py::test_remote_write_throttling_reload FAIL cidb, issue ISSUE EXISTS

@clickhouse-gh clickhouse-gh bot added the pr-improvement Pull request with some product improvements label Jan 2, 2026
@alexey-milovidov alexey-milovidov self-assigned this Jan 2, 2026
@pamarcos pamarcos added this pull request to the merge queue Jan 5, 2026
pamarcos added a commit to ClickHouse/clickhouse-docs that referenced this pull request Jan 5, 2026
@pamarcos
Update profiling-clickhouse-with-llvm-xray.mdx
9e50dfb 
Update doc after ClickHouse/ClickHouse#93345 syntax update
@pamarcos pamarcos mentioned this pull request Jan 5, 2026
Merged
3 tasks
Merged via the queue into ClickHouse:master with commit 6299407 Jan 5, 2026
127 of 131 checks passed
@pamarcos pamarcos deleted the system-instrument-ux-improvements branch January 5, 2026 09:29
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jan 5, 2026
@robot-ch-test-poll robot-ch-test-poll added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jan 5, 2026
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 mentioned this pull request Jan 5, 2026
Merged
robot-clickhouse-ci-1 added a commit that referenced this pull request Jan 5, 2026
@robot-clickhouse-ci-1
Merge pull request #93394 from ClickHouse/cherrypick/25.12/93345
89232b5 
Cherry pick #93345 to 25.12: Improve the UX of SYSTEM INSTRUMENT ADD/REMOVE
robot-clickhouse added a commit that referenced this pull request Jan 5, 2026
@robot-clickhouse
Backport #93345 to 25.12: Improve the UX of SYSTEM INSTRUMENT ADD/REMOVE
4bca5c0
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 mentioned this pull request Jan 5, 2026
Merged
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jan 5, 2026
pamarcos added a commit that referenced this pull request Jan 5, 2026
@pamarcos
Merge pull request #93395 from ClickHouse/backport/25.12/93345
c8d2187 
Backport #93345 to 25.12: Improve the UX of SYSTEM INSTRUMENT ADD/REMOVE
@pamarcos
Copy link
Copy Markdown
Member Author

pamarcos commented Jan 5, 2026 •
edited
Loading

Patching all functions that match broke the tests in private. The CI didn't catch it because the CH Inc sync job only runs the amd_asan version, which is built without XRay support because symbols clash and thus the tests are skipped.

Turns out in private when adding an instrumentation point for QueryMetricLog::queryStart it now patches two different functions: one is the original method and the other one is a lambda within it:

2026.01.05 12:06:38.252813 [ 212070 ] {937bffd8-bac5-42cc-9961-7e65a6ebfecd} <Information> InstrumentationManager: Adding instrumentation point for id 740, function_id 205671, function_name 'QueryMetr
icLog::startQuery', handler_name log, entry_type Exit, symbol DB::QueryMetricLog::startQuery(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000000l>>>, unsigned long), parameters [exit_three]                                           2026.01.05 12:06:38.252826 [ 212070 ] {937bffd8-bac5-42cc-9961-7e65a6ebfecd} <Information> InstrumentationManager: Adding instrumentation point for id 741, function_id 205689, function_name 'QueryMetr
icLog::startQuery', handler_name log, entry_type Exit, symbol void std::__1::__function::__policy_func<void ()>::__call_func[abi:ne210105]<DB::QueryMetricLog::startQuery(std::__1::basic_string<char, s
td::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000000l>>>, unsig
ned long)::$_0>(std::__1::__function::__policy_storage const*), parameters [exit_three]

I'm going to tune the logic a little bit in a following PR to avoid matching the function name if it's within the template arguments to avoid this sort of inconveniences in the future.

@pamarcos pamarcos mentioned this pull request Jan 5, 2026
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Assignees

@alexey-milovidov alexey-milovidov

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-improvement Pull request with some product improvements pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo v25.12-must-backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pamarcos @alexey-milovidov @robot-ch-test-poll @robot-ch-test-poll2 @robot-clickhouse-ci-1