Improve the observability of INSERT on distributed table by FrankChen021 · Pull Request #41034 · ClickHouse/ClickHouse

FrankChen021 · 2022-09-06T08:29:54Z

Changelog category (leave one):

Improvement

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

Improve the observability of INSERT on distributed table

Current INSERT on distributed is executed asynchronously. If the ASYNC insert from distributed table to local table fails, we don't know about that unless we check the text logs on the server. For a managed database service, it's not a good way for users to check such errors.

This PR first add more dimensions/metrics to the header of temp file that are going to be sent to remote tables, including:

cluser name
the database of distribtued table
name of target table
to which shard the data is written
how many rows and bytes the data is written to each shard

And then above information is written into opentelemetry span logs if opentelemetry tracing is enabled.
From the log, it's very clear that how the raw INSERT is splitted into different INSERTs on different shards.

The reason that we write these info to span logs is that we can easily extract these dimension/metrics from span logs with some existing tools, and then visualize these metrics for users.
Following is a demonstration that how we do this based on above information in the span logs.

src/Storages/Distributed/DirectoryMonitor.cpp

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh

Signed-off-by: Frank Chen <[email protected]>

src/Storages/Distributed/DirectoryMonitor.cpp

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh

Co-authored-by: Azat Khuzhin <[email protected]>

FrankChen021 · 2022-09-08T09:37:06Z

The problem is that test_cluster_two_shards consists from the same node - 127.1 and 127.2 (which is the same clickhouse instance), and so it executes the query, however runner waits until there will be two finished hosts.

So you can simply remove ON CLUSTER queries from you test, since it is not required there and everything will work.

Thanks for the explanation. I have not dived into the details of test.
I thought these are two nodes, so I set up this test cluster on my local with two nodes. If these two are the same clickhouse isntance, will the INSERT on shard 0 (or shard 1) be executed asynchronously - that means not call the writeToLocal instead of writing to a file first and then send the file to remote? I'm not sure about it.

azat · 2022-09-08T09:44:48Z

I thought these are two nodes

This is the cluster that is used - https://github.com/ClickHouse/ClickHouse/blob/master/programs/server/config.xml#L827-L840

so I set up this test cluster on my local with two nodes

You can simply use clickhouse-server -C /path/to/config.xml to use the same (but also there is a bunch of overrides in tests/config but AFAICS it is not important for you test)

If these two are the same clickhouse isntance, will the INSERT on shard 0 (or shard 1) be executed asynchronously - that means not call the writeToLocal instead of writing to a file first and then send the file to remote? I'm not sure about it.

127.1 - detected as localhost
127.2 - does not detected as localhost

That said that there will be one local node and one remote, just what you test needs.

Signed-off-by: Frank Chen <[email protected]>

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh

azat · 2022-09-09T12:26:53Z

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh

+#
+${CLICKHOUSE_CLIENT} -nq "
+DROP TABLE IF EXISTS ${CLICKHOUSE_DATABASE}.dist_opentelemetry;
+DROP TABLE IF EXISTS ${CLICKHOUSE_DATABASE}.local_opentelemetry;


You can omit CLICKHOUSE_DATABASE in CREATE/DROP, though it is minor.

Actually, when I remove the database from the the CREATE/DROP, the tables are not correctly created under the ${CLICKHOUSE_DATABASE} database but under default database. We should add --database to the command so that correct database will be applied, so I didn't make change to these queries.

This is done automatically by shell_config.sh -

ClickHouse/tests/queries/shell_config.sh

Line 21 in 9c5f9f1

[ -v CLICKHOUSE_DATABASE ] && CLICKHOUSE_CLIENT_OPT0+=" --database=${CLICKHOUSE_DATABASE} "

So this should work, but like I said this is minor, and if you don't have any other changes you can ignore this. But if you have something else, then please apply this suggestion.

Signed-off-by: Frank Chen <[email protected]>

FrankChen021 · 2022-09-12T06:19:39Z

127.1 - detected as localhost

127.2 - does not detected as localhost

That said that there will be one local node and one remote, just what you test needs.

Hi @azat

Last question, how does the CI setup the ClickHouse instance to listen on both 127.0.0.1 and 127.0.0.2?

I created a loopback 127.0.0.2 by using ifconfig lo0 alias 127.0.0.2 command on my MacBook, but the INSRT on distributed table treats both 127.0.01 and 127.0.0.2 as local nodes. However from the test result of CI, the 127.0.0.2 is treated as remote node.

azat · 2022-09-12T06:49:39Z

Last question, how does the CI setup the ClickHouse instance to listen on both 127.0.0.1 and 127.0.0.2?

From https://en.wikipedia.org/wiki/Loopback:

Various Internet Engineering Task Force (IETF) standards reserve the IPv4 address block 127.0.0.0/8, in CIDR notation and the IPv6 address ::1/128 for this purpose. The most common IPv4 address used is 127.0.0.1. Commonly these loopback addresses are mapped to the hostnames localhost or loopback.

So listening on 0.0.0.0/:: is enough.

I created a loopback 127.0.0.2 by using ifconfig lo0 alias 127.0.0.2 command on my MacBook

You don't need to create anything this should out of the box.
But, apparently on macos it does not work, since loopback interface has 127.0.0.1 while on linux 127.0.0.1/8

but the INSRT on distributed table treats both 127.0.01 and 127.0.0.2 as local nodes.

Yes, this is because it finds 127.2 on some interface locally and treat it as localhost because of this -

ClickHouse/src/Common/isLocalAddress.cpp

Lines 114 to 115 in 9c5f9f1

    
           NetworkInterfaces interfaces; 
        
           return interfaces.hasAddress(address);

If you have macos setup, then I would use test_cluster_two_shards_localhost and add two queries, with prefer_localhost_replica=0 and with prefer_localhost_replica=1 (default). That way you will cover both cases explicitly.

FrankChen021 · 2022-09-12T07:28:38Z

If you have macos setup, then I would use test_cluster_two_shards_localhost and add two queries, with prefer_localhost_replica=0 and with prefer_localhost_replica=1 (default). That way you will cover both cases explicitly.

Ah, I almost forget we have this setting prefer_localhost_replica. Thanks for reminder.

Signed-off-by: Frank Chen <[email protected]>

FrankChen021 · 2022-09-13T02:31:29Z

Looks like the building failure is not related to changes in this PR.

rschu1ze · 2022-09-13T08:41:57Z

Agree. Also the stress test failure looks unrelated to me. Should be good to merge once the remaining tests finished.

azat · 2023-01-21T17:14:25Z

src/Storages/Distributed/DirectoryMonitor.cpp

+        thread_trace_context->root_span.addAttribute("clickhouse.distributed", distributed_header.distributed_table);
+        thread_trace_context->root_span.addAttribute("clickhouse.remote", distributed_header.remote_table);
+        thread_trace_context->root_span.addAttribute("clickhouse.rows", distributed_header.rows);
+        thread_trace_context->root_span.addAttribute("clickhouse.bytes", distributed_header.bytes);


You forgot to adjust the code path for distributed_directory_monitor_batch_inserts - processFilesWithBatching, see 00e3c21

Also I was wondering, maybe it worth to use clickhouse.distributed_send. namespace for the metrics?

FrankChen021 added 6 commits September 5, 2022 16:37

Add cluster/distributed/remote to file

3d65e3f

Save cluster/distributed/table to log

a17bc51

Update writeToLocal to record related info

6ab1549

Remove extra attribute

8365e7b

Add test cases

49556da

Optimize span log for SYNC insert

2067096

robot-clickhouse added the pr-improvement Pull request with some product improvements label Sep 6, 2022

evillique added the can be tested Allows running workflows for external contributors label Sep 6, 2022

Use sleep to wait for flush

f21ab12

This comment was marked as outdated.

Sign in to view

rschu1ze self-assigned this Sep 7, 2022

This comment was marked as outdated.

Sign in to view

rschu1ze reviewed Sep 7, 2022

View reviewed changes

src/Storages/Distributed/DirectoryMonitor.cpp Show resolved Hide resolved

src/Storages/Distributed/DirectoryMonitor.cpp Outdated Show resolved Hide resolved

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

Address review comments

329f31e

Signed-off-by: Frank Chen <[email protected]>

rschu1ze approved these changes Sep 8, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

azat reviewed Sep 8, 2022

View reviewed changes

src/Storages/Distributed/DirectoryMonitor.cpp Outdated Show resolved Hide resolved

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh Outdated Show resolved Hide resolved

azat reviewed Sep 8, 2022

View reviewed changes

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh Outdated Show resolved Hide resolved

azat reviewed Sep 8, 2022

View reviewed changes

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh Outdated Show resolved Hide resolved

azat reviewed Sep 8, 2022

View reviewed changes

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

Update src/Storages/Distributed/DirectoryMonitor.cpp

a986380

Co-authored-by: Azat Khuzhin <[email protected]>

This comment was marked as outdated.

Sign in to view

FrankChen021 added 2 commits September 9, 2022 12:02

Improve test

237abff

Signed-off-by: Frank Chen <[email protected]>

Simplify test

92a92ba

Signed-off-by: Frank Chen <[email protected]>

FrankChen021 requested a review from azat September 9, 2022 04:20

Update test case

2fb0ae7

azat reviewed Sep 9, 2022

View reviewed changes

tests/queries/0_stateless/02417_opentelemetry_insert_on_distributed_table.sh Outdated Show resolved Hide resolved

azat reviewed Sep 9, 2022

View reviewed changes

FrankChen021 added 2 commits September 12, 2022 11:04

Compare content instead of count for easier problem solving

27b6a25

Signed-off-by: Frank Chen <[email protected]>

Fix testcase

16975ff

Signed-off-by: Frank Chen <[email protected]>

FrankChen021 added 4 commits September 12, 2022 22:15

Fix flaky tests

ebaa24e

Signed-off-by: Frank Chen <[email protected]>

Fix

7d6903b

Signed-off-by: Frank Chen <[email protected]>

Fix

7e1f290

Signed-off-by: Frank Chen <[email protected]>

Fix style

2019193

Signed-off-by: Frank Chen <[email protected]>

rschu1ze merged commit e8e6ddd into ClickHouse:master Sep 13, 2022

azat mentioned this pull request Jan 21, 2023

[RFC] Rewrite distributed sends to avoid using filesystem as a queue, use in-memory queue instead #45491

Merged

azat reviewed Jan 21, 2023

View reviewed changes

Conversation

FrankChen021 commented Sep 6, 2022

Changelog category (leave one):

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

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

FrankChen021 commented Sep 8, 2022

Uh oh!

azat commented Sep 8, 2022

Uh oh!

This comment was marked as outdated.

Uh oh!

azat Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

azat Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

FrankChen021 commented Sep 12, 2022

Uh oh!

azat commented Sep 12, 2022

Uh oh!

FrankChen021 commented Sep 12, 2022

Uh oh!

FrankChen021 commented Sep 13, 2022

Uh oh!

rschu1ze commented Sep 13, 2022

Uh oh!

azat Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

azat Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants