Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[feat] Store traces in ClickHouse based on Jaeger V2 #6725

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

[feat] Store traces in ClickHouse based on Jaeger V2 #6725

wants to merge 3 commits into from
+2,695 −16

Conversation

zhengkezhou1
Copy link
Contributor

@zhengkezhou1 zhengkezhou1 commented Feb 13, 2025 •
edited
Loading

Which problem is this PR solving?

Desgin doc: Jaeger V2: Support for ClickHouse as Storage Backend
Part of #5058

Description of the changes

  • Introduce clickhouse client: ch-go & clickhouse-go v2 to be used for writing/reading traces
  • Provide test container environment for ClickHouse

How was this change tested?

  • unit tests & intergation tests

Checklist

@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch from a65794d to bacbf97 Compare February 13, 2025 13:51
@codecov Codecov
Copy link

codecov bot commented Feb 13, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.84117% with 38 lines in your changes missing coverage. Please review.

Project coverage is 96.10%. Comparing base (19b9749) to head (6092508).

Files with missing lines Patch % Lines
internal/storage/v2/clickhouse/factory.go 77.77% 8 Missing and 4 partials ⚠️
internal/storage/v2/clickhouse/schema/schema.go 83.01% 6 Missing and 3 partials ⚠️
internal/storage/v2/clickhouse/wrapper/wrapper.go 86.76% 6 Missing and 3 partials ⚠️
...nternal/storage/v2/clickhouse/tracestore/reader.go 88.88% 6 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6725      +/-   ##
==========================================
- Coverage   96.25%   96.10%   -0.16%     
==========================================
  Files         342      350       +8     
  Lines       20291    20908     +617     
==========================================
+ Hits        19532    20094     +562     
- Misses        574      617      +43     
- Partials      185      197      +12
Flag Coverage Δ
badger_v1 9.52% <0.00%> (-0.69%) ⬇️
badger_v2 2.11% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1-manual 14.37% <0.00%> (-0.99%) ⬇️
cassandra-4.x-v2-auto 2.10% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2-manual 2.10% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1-manual 14.37% <0.00%> (-0.99%) ⬇️
cassandra-5.x-v2-auto 2.10% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v2-manual 2.10% <0.00%> (-0.04%) ⬇️
clickhouse-25.x 5.48% <69.20%> (?)
elasticsearch-6.x-v1 0.26% <0.00%> (-19.86%) ⬇️
elasticsearch-7.x-v1 0.26% <0.00%> (-19.95%) ⬇️
elasticsearch-8.x-v1 0.26% <0.00%> (-20.12%) ⬇️
elasticsearch-8.x-v2 2.11% <0.00%> (-0.01%) ⬇️
grpc_v1 10.53% <0.00%> (-0.75%) ⬇️
grpc_v2 8.23% <0.00%> (-0.01%) ⬇️
kafka-3.x-v1 9.81% <0.00%> (-0.71%) ⬇️
kafka-3.x-v2 2.11% <0.00%> (-0.01%) ⬇️
memory_v2 2.11% <0.00%> (-0.01%) ⬇️
opensearch-1.x-v1 19.05% <0.00%> (-1.21%) ⬇️
opensearch-2.x-v1 19.05% <0.00%> (-1.21%) ⬇️
opensearch-2.x-v2 2.11% <0.00%> (-0.01%) ⬇️
tailsampling-processor 0.57% <0.00%> (-0.01%) ⬇️
unittests 94.54% <78.44%> (-0.51%) ⬇️

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 zhengkezhou1 force-pushed the write-path-for-clickhouse branch from bacbf97 to 21c3fab Compare February 13, 2025 14:30
@zhengkezhou1 zhengkezhou1 changed the title [feat][storage]: add write path for ClickHouse based on Jaeger V2 [feat][storage] add write path for ClickHouse based on Jaeger V2 Feb 13, 2025
@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch 5 times, most recently from a16370b to 72d91cc Compare February 14, 2025 07:24
@zhengkezhou1
Copy link
Contributor Author

zhengkezhou1 commented Feb 14, 2025 •
edited
Loading

@yurishkuro ClickHouse integration test not working in CI. Is there anything I might have missed?

@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch 2 times, most recently from 51b3f8f to b8915ef Compare February 14, 2025 10:52
@zhengkezhou1 zhengkezhou1 marked this pull request as ready for review February 14, 2025 11:00
@zhengkezhou1 zhengkezhou1 requested a review from a team as a code owner February 14, 2025 11:00
@dosubot dosubot bot added area/storage docker Pull requests that update Docker code v2 labels Feb 14, 2025
@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch from 572c1a2 to 7718d65 Compare February 15, 2025 11:31
@zhengkezhou1 zhengkezhou1 changed the title [feat][storage] add write path for ClickHouse based on Jaeger V2 [feat][storage] Store Traces in ClickHouse Based on Jaeger V2 Feb 15, 2025
@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch 2 times, most recently from 19bc811 to c408a25 Compare February 15, 2025 12:43
@zhengkezhou1
Copy link
Contributor Author

I would like to implement all basic features, ensure that the integration tests pass, and then return to address other low-priority tasks.

yurishkuro
yurishkuro reviewed Feb 16, 2025
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

before moving code around I suggest you read & understand the comments and then propose a new directory structure that we can agree on. This will reduce the churn.

@zhengkezhou1
Copy link
Contributor Author

zhengkezhou1 commented Feb 18, 2025 •
edited
Loading

@yurishkuro The key here is that the design of the ch-go and clickhouse-go clients are very different, making it difficult to abstract them. Therefore, it's best to isolate them and use the minimal configuration file required for initialization as follows:

schema:
  #auto run DDL script
  auto: true
client:
  database: jaeger
  username: default
  password: default
  #ch-go
  writer:
    address: "127.0.0.1:9200"
    pool:
      max_connection_lifetime: 3600000000000
      max_connection_idle_time: 1800000000000
      #CPU Core number
      min_connections: 4
      #CPU Core number * 2
      max_connections: 8
      health_check_period: 60000000000
  #clickhouse-go
  reader:
    #no cluster just a different field here.
    addresses: ["node00:9200","node01:9200","node02:9200"]

The directory structure has been adjusted as follows:

internal/storage/v2/clickhouse/

  • client
    • conn Create a clickhouse-go connection.
    • pool Create ch-go connection pool.
  • config All configuration items correspond to the configuration file.
  • internal Tools required to write traces into the database.
  • wrapper Wrapper to isolate upper-layer calls from third-party implementations.
  • schema Initialization DLL scripts and providing automatic database initialization functionality.
  • tracestore Implement read and write operations for traces.

@zhengkezhou1
Copy link
Contributor Author

@yurishkuro In the implementation of GetTraces(ctx context.Context, traceIDs ...GetTraceParams),note that only the TraceID is utilized among the param in GetTraceParams. are these employed for range queries?

@yurishkuro
Copy link
Member

Code pointer?

@zhengkezhou1
Copy link
Contributor Author

zhengkezhou1 commented Feb 22, 2025 •
edited
Loading

Code pointer?

jaeger/internal/storage/v1/cassandra/spanstore/reader.go

Lines 150 to 154 in 84212d2

ctx, span := s.startSpanForQuery(ctx, "readTrace", querySpanByTraceID)
defer span.End()
span.SetAttributes(attribute.Key("trace_id").String(traceID.String()))
trc, err := s.readTraceInSpan(ctx, traceID)

And only gRPC really use them all.

jaeger/internal/storage/v1/grpc/shared/grpc_client.go

Lines 72 to 76 in 84212d2

stream, err := c.readerClient.GetTrace(ctx, &storage_v1.GetTraceRequest{
TraceID: query.TraceID,
StartTime: query.StartTime,
EndTime: query.EndTime,
})

Also, do you think the new code structure I proposed is suitable?

@yurishkuro
Copy link
Member

The timestamps in GetTraces request were introduced on request from 3rd party implementations (e.g. Tempo). None of the internally supported backends need these parameters because of how the db schemas are organized. But it was expected that the timestamps could be useful for ClickHouse.

@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch from 6981216 to 8a35cbc Compare March 4, 2025 14:17
@zhengkezhou1 zhengkezhou1 marked this pull request as ready for review March 4, 2025 14:45
@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch from 8a35cbc to a1edda1 Compare March 5, 2025 08:05
@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch 9 times, most recently from 9868dd3 to e62882d Compare March 11, 2025 12:27
@zhengkezhou1 zhengkezhou1 marked this pull request as draft March 11, 2025 15:58
zhengkezhou1
zhengkezhou1 commented Mar 11, 2025
View reviewed changes
Copy link
Contributor Author

@zhengkezhou1 zhengkezhou1 left a comment

Choose a reason for hiding this comment

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

@yurishkuro Can you please take a look at my new improvements and ideas?

@zhengkezhou1
Copy link
Contributor Author

Hi @yurishkuro, I just wanted to ask if anything is blocking this. I found this PR too large—maybe it would be better to split it into smaller ones?

yurishkuro
yurishkuro reviewed Mar 20, 2025
View reviewed changes
internal/storage/v2/clickhouse/client/client.go Outdated
"go.opentelemetry.io/collector/pdata/ptrace"
)

type Pool interface {
Copy link
Member

Choose a reason for hiding this comment

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

why is it called Pool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's challenging to name here, so this time I'll directly use the package names: ch-go → chpool, clickhouse-go → clickhouse.

@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch 2 times, most recently from 4f0e116 to d37c2ad Compare March 25, 2025 00:15
@zhengkezhou1
[feat] Store traces in ClickHouse based on Jaeger V2
6ec305e 
Signed-off-by: zhengkezhou1 <[email protected]>
@zhengkezhou1
improve
56d19b7 
Signed-off-by: zhengkezhou1 <[email protected]>
@zhengkezhou1
e2e tests will fail
6092508 
Signed-off-by: zhengkezhou1 <[email protected]>
@zhengkezhou1 zhengkezhou1 force-pushed the write-path-for-clickhouse branch from 1478547 to 6092508 Compare March 25, 2025 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

Copilot code review Copilot Copilot left review comments

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

@albertteoh albertteoh Awaiting requested review from albertteoh albertteoh is a code owner automatically assigned from jaegertracing/jaeger-maintainers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/storage docker Pull requests that update Docker code v2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zhengkezhou1 @yurishkuro