[clickhouse] Introduce trace writer for ClickHouse storage #7141

zhengkezhou1 · 2025-05-18T14:27:23Z

Which problem is this PR solving?

Resolves [clickhouse] Implement Trace Writer (Exporter) For ClickHouse Storage #7135

Description of the changes

Extract the API used for batch writing from clickhouse-go into an interface and inject it into tracewriter. The benefits are Decoupling of business logic: tracewriter should not be concerned with any ClickHouse connection-related details.
More convenient unit testing through mocking, without the need for a real ClickHouse server Implement the basic logic of the writeTraces method (without involving actual data writing).

How was this change tested?

unit test.

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: npm run lint and npm run test

zhengkezhou1 · 2025-05-18T14:33:53Z

internal/storage/v2/clickhouse/tracestore/writer.go

+
+	for range traces {
+		// TODO Maybe AppendStruct is a better choice.
+		err := batch.Append()


This change is not complete. The batch.Append call should resemble the following:

err := batch.Append( spanRow.StartTime, spanRow.TraceId, spanRow.SpanId, spanRow.ParentSpanId, spanRow.TraceState, spanRow.ServiceName, spanRow.Name, spanRow.Kind, spanRow.Duration, spanRow.StatusCode, spanRow.StatusMessage, spanRow.Attributes.BoolKeys, spanRow.Attributes.BoolValues, spanRow.Attributes.DoubleKeys, spanRow.Attributes.DoubleValues, spanRow.Attributes.IntKeys, spanRow.Attributes.IntValues, spanRow.Attributes.StrKeys, spanRow.Attributes.StrValues, spanRow.Attributes.BytesKeys, spanRow.Attributes.BytesValues, scopeRow.Name, scopeRow.Version, scopeRow.Attributes.BoolKeys, scopeRow.Attributes.BoolValues, scopeRow.Attributes.DoubleKeys, scopeRow.Attributes.DoubleValues, scopeRow.Attributes.IntKeys, scopeRow.Attributes.IntValues, scopeRow.Attributes.StrKeys, scopeRow.Attributes.StrValues, scopeRow.Attributes.BytesKeys, scopeRow.Attributes.BytesValues, resourceRow.Attributes.BoolKeys, resourceRow.Attributes.BoolValues, resourceRow.Attributes.DoubleKeys, resourceRow.Attributes.DoubleValues, resourceRow.Attributes.IntKeys, resourceRow.Attributes.IntValues, resourceRow.Attributes.StrKeys, resourceRow.Attributes.StrValues, resourceRow.Attributes.BytesKeys, resourceRow.Attributes.BytesValues, eventsRow.Names, eventsRow.Timestamps, eventsAllAttrRow.BoolAttrs, eventsAllAttrRow.DoubleAttrs, eventsAllAttrRow.IntAttrs, eventsAllAttrRow.StrAttrs, eventsAllAttrRow.BytesAttrs, linksRow.TraceIds, linksRow.SpanIds, linksRow.TraceStates, linksAllAttrRow.BoolAttrs, linksAllAttrRow.DoubleAttrs, linksAllAttrRow.IntAttrs, linksAllAttrRow.StrAttrs, linksAllAttrRow.BytesAttrs, )

However, to keep this PR small and simple, adding the batch.Append logic with all these fields would be too large of a change. Therefore, it's better to add the complete batch.Append implementation in a subsequent PR.

codecov · 2025-05-18T14:35:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.22%. Comparing base (7d66726) to head (89fe792).
⚠️ Report is 230 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7141      +/-   ##
==========================================
+ Coverage   96.20%   96.22%   +0.01%     
==========================================
  Files         358      359       +1     
  Lines       21596    21708     +112     
==========================================
+ Hits        20777    20889     +112     
  Misses        613      613              
  Partials      206      206

Flag	Coverage Δ
badger_v1	`9.90% <ø> (ø)`
badger_v2	`2.05% <ø> (ø)`
cassandra-4.x-v1-manual	`14.89% <ø> (ø)`
cassandra-4.x-v2-auto	`2.04% <ø> (ø)`
cassandra-4.x-v2-manual	`2.04% <ø> (ø)`
cassandra-5.x-v1-manual	`14.89% <ø> (ø)`
cassandra-5.x-v2-auto	`2.04% <ø> (ø)`
cassandra-5.x-v2-manual	`2.04% <ø> (ø)`
elasticsearch-6.x-v1	`20.23% <ø> (ø)`
elasticsearch-7.x-v1	`20.31% <ø> (ø)`
elasticsearch-8.x-v1	`20.49% <ø> (ø)`
elasticsearch-8.x-v2	`2.05% <ø> (ø)`
grpc_v1	`11.44% <ø> (ø)`
grpc_v2	`2.05% <ø> (ø)`
kafka-3.x-v1	`10.17% <ø> (ø)`
kafka-3.x-v2	`2.05% <ø> (ø)`
memory_v2	`2.05% <ø> (ø)`
opensearch-1.x-v1	`20.36% <ø> (ø)`
opensearch-2.x-v1	`20.36% <ø> (ø)`
opensearch-2.x-v2	`2.05% <ø> (ø)`
query	`2.05% <ø> (ø)`
tailsampling-processor	`0.55% <ø> (ø)`
unittests	`95.05% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

zhengkezhou1 · 2025-05-18T14:35:47Z

internal/storage/v2/clickhouse/tracestore/writer.go

+	"github.com/jaegertracing/jaeger/internal/storage/v2/clickhouse/tracestore/dbmodel"
+)
+
+const insertTrace = "INSERT INTO otel_traces"


not complete yet; I am waiting for the schema to be defined.

Signed-off-by: zhengkezhou1 <[email protected]>

mahadzaryab1 · 2025-05-19T12:34:47Z

@zhengkezhou1 thanks for opening a PR! I'll take a look at it soon.

mahadzaryab1

a few questions - great start so far!

.mockery.yaml

mahadzaryab1 · 2025-05-20T01:07:36Z

internal/storage/v2/clickhouse/tracestore/writer.go

+	"github.com/jaegertracing/jaeger/internal/storage/v2/clickhouse/tracestore/dbmodel"
+)
+
+const insertTrace = "INSERT INTO otel_traces"


is there a reason we need to call the table otel_traces? why not just traces?

no, use traces is ok.

mahadzaryab1 · 2025-05-20T01:09:40Z

internal/storage/v2/clickhouse/tracestore/writer_test.go

+		err := writer.WriteTraces(context.Background(), ptrace.NewTraces())
+		require.ErrorContains(t, err, "connect to server timeout")
+	})
+	t.Run("failed when call Send not works", func(t *testing.T) {


can we either turn these into table driven tests or make them separate unit tests?

sure, use the table driven test.

mahadzaryab1 · 2025-05-20T01:14:25Z

internal/storage/v2/clickhouse/tracestore/writer.go

+	// traces now using the proto.Input to write, it should be replaced once chpool has been dropped.
+	// see https://github.com/jaegertracing/jaeger/pull/7093


would it better to make this change first?

Yes, i create a new PR: #7149 for it.

Signed-off-by: zhengkezhou1 <[email protected]>

github-actions · 2025-07-28T02:16:36Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time.

github-actions · 2025-10-06T02:07:26Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time.

zhengkezhou1 requested a review from a team as a code owner May 18, 2025 14:27

zhengkezhou1 requested a review from mahadzaryab1 May 18, 2025 14:27

dosubot bot added area/storage v2 labels May 18, 2025

zhengkezhou1 commented May 18, 2025

View reviewed changes

feat(storage): Introduce trace writer for clickhouse

a76fc5b

Signed-off-by: zhengkezhou1 <[email protected]>

zhengkezhou1 force-pushed the feat/clickhouse-writer branch from ec3635b to a76fc5b Compare May 18, 2025 14:41

add package test

ce7cb01

Signed-off-by: zhengkezhou1 <[email protected]>

mahadzaryab1 added the changelog:exprimental Change to an experimental part of the code label May 19, 2025

mahadzaryab1 changed the title ~~feat(storage): Introduce trace writer for clickhouse~~ [clickhouse] Introduce trace writer for ClickHouse storage May 20, 2025

mahadzaryab1 reviewed May 20, 2025

View reviewed changes

table driven tests

89fe792

Signed-off-by: zhengkezhou1 <[email protected]>

github-actions bot added the stale The issue/PR has become stale and may be auto-closed label Jul 28, 2025

github-actions bot removed the stale The issue/PR has become stale and may be auto-closed label Aug 4, 2025

github-actions bot added the stale The issue/PR has become stale and may be auto-closed label Oct 6, 2025

		// traces now using the proto.Input to write, it should be replaced once chpool has been dropped.
		// see https://github.com/jaegertracing/jaeger/pull/7093

[clickhouse] Introduce trace writer for ClickHouse storage #7141

Are you sure you want to change the base?

[clickhouse] Introduce trace writer for ClickHouse storage #7141

Uh oh!

Conversation

zhengkezhou1 commented May 18, 2025

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahadzaryab1 commented May 19, 2025

Uh oh!

mahadzaryab1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 28, 2025

Uh oh!

github-actions bot commented Oct 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented May 18, 2025 •

edited

Loading