[WIP] Implement spans exporting for ClickHouse storage in Jaeger V2 #4941

haanhvu · 2023-11-12T15:18:45Z

Which problem is this PR solving?

Part of #5058

haanhvu · 2023-11-12T15:19:44Z

@yurishkuro This still needs some supplements and a lot of cleanups and tests. I'm still working on them. But could you take an initial look to see if I'm going the right way? Thanks!

cmd/jaeger/internal/extension/jaegerstorage/config.go

yurishkuro · 2023-11-13T04:01:30Z

plugin/storage/clickhouse/factory.go

+	//TODO: Move some steps to Initialize()
+}
+
+func (f *Factory) CreateSpansTable(ctx context.Context) {


why is this function public, who would call it?

Currently, Jaeger V2's exporter checks the type of Factory. If it's a clickhouse Factory, then f.CreateSpansTable() is called, as you can see in this piece.

yurishkuro · 2023-11-13T04:01:57Z

plugin/storage/clickhouse/factory.go

+	}
+}
+
+func (f *Factory) ExportSpans(ctx context.Context, td ptrace.Traces) error {


why is ExportSpans defined on the factory instead of span store?

The logic is defined in plugin/storage/clickhouse/spanstore/exporter.go (link). But Jaeger V2's exporter is depending on the real type of Factory to decide whether to call SpanWriter.WriteSpan() or ExportSpans() (link). So I think making ExportSpans() a func of clickhouse Factory may be a good idea.

Of course, a better idea is to define a new API for ExportSpans(), but maybe we can do that later? Or you have a better idea?

I have a better idea, but it is not well formed yet, I need to write it down and maybe put together the new interfaces. Will try to do this later this weekend. Are you blocked on it?

No I don't think this blocks the progress. Can continue.

k0zl · 2023-11-29T18:41:30Z

👍 I've been waiting clickhouse support for a long time. Is there anything I can help you with?

haanhvu · 2023-11-30T05:55:34Z

👍 I've been waiting clickhouse support for a long time. Is there anything I can help you with?

@k0zl Sure! For this PR (spans exporting), the remaining parts are:

Finish schema implementation in plugin/storage/clickhouse/spanstore/schema.go. We described our schema choices in our benchmark report. As you can see in the current schema.go, I just implemented the spans table, not the materialized views yet.
Add tests: I'm pushing an unit factory_test.go this week. There are other unit tests, e2e test, or even local test with Jaeger v2 binary to do. (Jaeger v2 issue/decription is here)

After finishing with spans exporting, we'll move to spans reading,...

Also, since clickhouse storage is supported in Jaeger v2, to make this realeased you could also help with Jaeger v2 roadmap, in the issue I linked above.

Please choose whatever interests you. Thanks!

yurishkuro · 2023-11-30T16:03:41Z

@haanhvu it would be useful to write a design doc / roadmap documenting what we're doing and what tasks need to be done, are you up for that? You can model it after the Jaeger V2 document.

haanhvu · 2023-11-30T16:52:51Z

@haanhvu it would be useful to write a design doc / roadmap documenting what we're doing and what tasks need to be done, are you up for that? You can model it after the Jaeger V2 document.

Yeah I'll create an issue for supporting clickhouse in jaeger v2 tomorrow

codecov · 2023-12-07T14:37:13Z

Codecov Report

Attention: 105 lines in your changes are missing coverage. Please review.

Comparison is base (d489e3a) 95.62% compared to head (f7c95ed) 95.08%.

Files	Patch %	Lines
plugin/storage/clickhouse/spanstore/exporter.go	0.00%	67 Missing ⚠️
plugin/storage/clickhouse/config.go	58.13%	12 Missing and 6 partials ⚠️
plugin/storage/clickhouse/factory.go	45.16%	13 Missing and 4 partials ⚠️
plugin/storage/clickhouse/spanstore/schema.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4941      +/-   ##
==========================================
- Coverage   95.62%   95.08%   -0.54%     
==========================================
  Files         314      318       +4     
  Lines       18294    18438     +144     
==========================================
+ Hits        17493    17532      +39     
- Misses        642      737      +95     
- Partials      159      169      +10

Flag	Coverage Δ
cassandra-3.x	`25.60% <ø> (ø)`
cassandra-4.x	`25.60% <ø> (ø)`
elasticsearch-5.x	`19.88% <ø> (ø)`
elasticsearch-6.x	`19.88% <ø> (ø)`
elasticsearch-7.x	`20.01% <ø> (ø)`
elasticsearch-8.x	`20.10% <ø> (ø)`
grpc-badger	`19.49% <ø> (ø)`
kafka	`14.10% <ø> (ø)`
opensearch-1.x	`20.01% <ø> (ø)`
opensearch-2.x	`20.00% <ø> (-0.02%)`	⬇️
unittests	`92.78% <27.08%> (-0.52%)`	⬇️

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.

haanhvu · 2023-12-29T17:04:52Z

@yurishkuro Could you take a look at the failed all-in-one build? make all-in-one-integration-test passed locally for me. Not sure why it fails in CI.

yurishkuro · 2023-12-29T17:24:14Z

I am seeing error

Error: failed to build pipelines: failed to create "jaeger_storage_exporter" exporter for data type "traces": nil PushTraces
2023/12/29 16:57:59 failed to build pipelines: failed to create "jaeger_storage_exporter" exporter for data type "traces": nil PushTraces
+ exit 1

yurishkuro · 2023-12-29T17:45:10Z

cmd/jaeger/internal/exporters/storageexporter/exporter.go

+	config             *Config
+	logger             *zap.Logger
+	spanWriter         spanstore.Writer
+	exportTraces       func(ctx context.Context, td ptrace.Traces) error


nit: I would call this consumeTraces, because the argument that we pass to the exporter helper is consumer.ConsumeTracesFunc

cmd/jaeger/internal/exporters/storageexporter/factory.go

yurishkuro · 2023-12-29T17:48:22Z

cmd/jaeger/internal/exporters/storageexporter/exporter.go

+			return fmt.Errorf("cannot create span writer: %w", err)
+		}
+		exp.requireBatchInsert = false
+		exp.exportTraces = exp.pushTraces


nit: I'd rename s/pushTraces/writeSpans/ to indicate jaeger-v1 style storage

cmd/jaeger/internal/exporters/storageexporter/exporter.go

haanhvu · 2023-12-29T17:50:43Z

I am seeing error

Error: failed to build pipelines: failed to create "jaeger_storage_exporter" exporter for data type "traces": nil PushTraces
2023/12/29 16:57:59 failed to build pipelines: failed to create "jaeger_storage_exporter" exporter for data type "traces": nil PushTraces
+ exit 1

@yurishkuro This error didn't come up in my make all-in-one-integration-test. Probably because my make all-in-one-integration-test checked v1, not v2? How to make sure this command checks v2?

yurishkuro · 2023-12-29T20:50:57Z

This error didn't come up in my make all-in-one-integration-test

This test doesn't just run on its own, it expects the binary running, so how are you starting the binary?

For example, this works:

# terminal 1
$ go run -tags=ui ./cmd/jaeger

# terminal 2
$ SKIP_SAMPLING=true make all-in-one-integration-test

But this is running with default aio config for OTEL, whereas you need to provide your own config in order to test the same against CH.

haanhvu · 2024-01-01T08:43:53Z

go run -tags=ui ./cmd/jaeger

Ah, I ran the binary with make run-all-in-one... That's why v2 wasn't checked... Let me try again.

haanhvu · 2024-01-01T15:51:10Z

But this is running with default aio config for OTEL, whereas you need to provide your own config in order to test the same against CH.

The nil Pushtraces error with default aio config has been resolved.

So now I'll have to run clickhouse server, make my own jaeger v2 config (that exports to clickhouse) and go run -tags=ui ./cmd/jaeger --config=config.yaml right? I'll try it tomorrow.

github-actions · 2024-01-02T18:22:15Z

Test Results

2 054 tests +2 2 044 ✅ +2 1m 9s ⏱️ ±0s
221 suites +2 10 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit f7c95ed. ± Comparison against base commit d489e3a.

♻️ This comment has been updated with latest results.

Signed-off-by: haanhvu <haanh6594@gmail.com>

haanhvu · 2024-01-02T20:24:28Z

@yurishkuro I ran the binary with my config.yaml, spans were exported successfully to clickhouse:

Should we add an e2e test now, or wait until spans reading are implemented?

Also, can you check the unit tests? I just have one factory_test.go, no test files for config.go or spanstore because factory already calls config and spanstore.

Pls check if there's anything else you think we should test in this PR.

yurishkuro · 2024-01-02T20:57:32Z

Should we add an e2e test now, or wait until spans reading are implemented?

You wouldn't be able to do e2e without reading, that's how the tests verify that traces are saved.

yurishkuro · 2024-01-02T20:59:40Z

cmd/jaeger/internal/exporters/storageexporter/exporter.go

+	switch t := f.(type) {
+	case *ch.Factory:
+		exp.clickhouse = true
+		t.CreateSpansTable(ctx)


why can't we do this inside clickhouse.NewFactory? storageexporter doesn't need to know about internal details of each storage implementation

haanhvu requested a review from a team as a code owner November 12, 2023 15:18

haanhvu requested a review from yurishkuro November 12, 2023 15:18

haanhvu changed the title ~~[WIP] Implement spans exporting for ClickHouse storage for Jaeger V2~~ [WIP] Implement spans exporting to ClickHouse storage for Jaeger V2 Nov 12, 2023

haanhvu changed the title ~~[WIP] Implement spans exporting to ClickHouse storage for Jaeger V2~~ [WIP] Implement spans exporting for ClickHouse storage in Jaeger V2 Nov 12, 2023

yurishkuro reviewed Nov 13, 2023

View reviewed changes

cmd/jaeger/internal/extension/jaegerstorage/config.go Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 13, 2023

View reviewed changes

haanhvu force-pushed the ch-pushtraces branch from 063ac74 to 4608c16 Compare November 18, 2023 16:51

haanhvu force-pushed the ch-pushtraces branch from 7010d65 to 5b69530 Compare December 7, 2023 14:19

haanhvu force-pushed the ch-pushtraces branch from 31cc9b7 to d780080 Compare December 29, 2023 16:26

haanhvu mentioned this pull request Dec 29, 2023

[Feature]: Support ClickHouse storage in Jaeger V2 #5058

Open

7 tasks

yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Dec 29, 2023

yurishkuro marked this pull request as draft December 29, 2023 17:16

yurishkuro reviewed Dec 29, 2023

View reviewed changes

haanhvu force-pushed the ch-pushtraces branch from 1482423 to a7616e6 Compare January 1, 2024 12:49

haanhvu force-pushed the ch-pushtraces branch from c0378b4 to 0de83b3 Compare January 2, 2024 18:13

Implement spans exporting in ClickHouse storage

fd9c731

Signed-off-by: haanhvu <haanh6594@gmail.com>

haanhvu force-pushed the ch-pushtraces branch from cb3e1d3 to fd9c731 Compare January 2, 2024 18:42

Fix idl submodule

f7c95ed

Signed-off-by: haanhvu <haanh6594@gmail.com>

yurishkuro reviewed Jan 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Implement spans exporting for ClickHouse storage in Jaeger V2 #4941

[WIP] Implement spans exporting for ClickHouse storage in Jaeger V2 #4941

haanhvu commented Nov 12, 2023 •

edited

haanhvu commented Nov 12, 2023 •

edited

yurishkuro Nov 13, 2023

haanhvu Nov 15, 2023

yurishkuro Nov 13, 2023

haanhvu Nov 15, 2023 •

edited

yurishkuro Nov 16, 2023

haanhvu Nov 16, 2023

k0zl commented Nov 29, 2023

haanhvu commented Nov 30, 2023 •

edited

yurishkuro commented Nov 30, 2023

haanhvu commented Nov 30, 2023

codecov bot commented Dec 7, 2023 •

edited

haanhvu commented Dec 29, 2023

yurishkuro commented Dec 29, 2023

yurishkuro Dec 29, 2023

yurishkuro Dec 29, 2023

haanhvu commented Dec 29, 2023 •

edited

yurishkuro commented Dec 29, 2023

haanhvu commented Jan 1, 2024 •

edited

haanhvu commented Jan 1, 2024 •

edited

github-actions bot commented Jan 2, 2024 •

edited

haanhvu commented Jan 2, 2024 •

edited

yurishkuro commented Jan 2, 2024

yurishkuro Jan 2, 2024

[WIP] Implement spans exporting for ClickHouse storage in Jaeger V2 #4941

Are you sure you want to change the base?

[WIP] Implement spans exporting for ClickHouse storage in Jaeger V2 #4941

Conversation

haanhvu commented Nov 12, 2023 • edited

Which problem is this PR solving?

haanhvu commented Nov 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haanhvu Nov 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k0zl commented Nov 29, 2023

haanhvu commented Nov 30, 2023 • edited

yurishkuro commented Nov 30, 2023

haanhvu commented Nov 30, 2023

codecov bot commented Dec 7, 2023 • edited

Codecov Report

haanhvu commented Dec 29, 2023

yurishkuro commented Dec 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haanhvu commented Dec 29, 2023 • edited

yurishkuro commented Dec 29, 2023

haanhvu commented Jan 1, 2024 • edited

haanhvu commented Jan 1, 2024 • edited

github-actions bot commented Jan 2, 2024 • edited

Test Results

haanhvu commented Jan 2, 2024 • edited

yurishkuro commented Jan 2, 2024

Choose a reason for hiding this comment

haanhvu commented Nov 12, 2023 •

edited

haanhvu commented Nov 12, 2023 •

edited

haanhvu Nov 15, 2023 •

edited

haanhvu commented Nov 30, 2023 •

edited

codecov bot commented Dec 7, 2023 •

edited

haanhvu commented Dec 29, 2023 •

edited

haanhvu commented Jan 1, 2024 •

edited

haanhvu commented Jan 1, 2024 •

edited

github-actions bot commented Jan 2, 2024 •

edited

haanhvu commented Jan 2, 2024 •

edited