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.

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

[jaeger-v2][storage] Add Configuration For ClickHouse #6154

Closed
mahadzaryab1 opened this issue Nov 3, 2024 · 11 comments
Closed

[jaeger-v2][storage] Add Configuration For ClickHouse #6154

mahadzaryab1 opened this issue Nov 3, 2024 · 11 comments

Comments

@mahadzaryab1
Copy link
Collaborator

mahadzaryab1 commented Nov 3, 2024

As a first step in adding support for ClickHouse, we want to define the configuration that Jaeger will expose for users to create a connection to ClickHouse. The configurable options can be viewed here.

@mahadzaryab1
Copy link
Collaborator Author

mahadzaryab1 commented Nov 3, 2024

@yurishkuro thoughts on if we should be using the native clickhouse interface through go-clickhouse or the standard database/sql package in Go to connect to clickhouse (ref: https://clickhouse.com/docs/en/integrations/go#clickhouse-go-client).

@mahadzaryab1 mahadzaryab1 changed the title [jaeger-v2][storage]Add Configuration For ClickHouse [jaeger-v2][storage] Add Configuration For ClickHouse Nov 3, 2024
@yurishkuro
Copy link
Member

I don't have an opinion, did not research this question. Note that we already have an implementation in #4941

@haanhvu
Copy link
Contributor

haanhvu commented Dec 10, 2024

@yurishkuro thoughts on if we should be using the native clickhouse interface through go-clickhouse or the standard database/sql package in Go to connect to clickhouse (ref: https://clickhouse.com/docs/en/integrations/go#clickhouse-go-client).

I thought of this when doing #4941 too. The reason I chose database/sql back then was because OTel ClickHouse exporter used it, and this exporter had been widely used and tested by the community: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/clickhouseexporter

@yurishkuro
Copy link
Member

According to the docs the sql/database API is the least performant of the three APIs, so we should avoid it. It would be interesting to see how more complicated the ch-go usage would be, as that's the most performant way, but the middle ground with clickhouse-go is probably ok.

Another option is to use ch-go for writing and clickhouse-go for reading.

Cc @mahadzaryab1

@adityachopra29
Copy link
Contributor

Working on this issue.

@LGDHuaOPER
Copy link

Good!

@adityachopra29
Copy link
Contributor

adityachopra29 commented Jan 24, 2025

According to the docs the sql/database API is the least performant of the three APIs, so we should avoid it. It would be interesting to see how more complicated the ch-go usage would be, as that's the most performant way, but the middle ground with clickhouse-go is probably ok.

Another option is to use ch-go for writing and clickhouse-go for reading.

Cc @mahadzaryab1

@yurishkuro Just wanted a confirmation as to what approach we will be taking for clickhouse.(the "ch-go for writing and clickhouse-go for reading." one or simply clickhouse-go one.
I have learned about the problem and the approach to storing data(ie as a single table) (as described here), about clickhouse, and was working on the plugin/storage/clickhouse/options.go.

@yurishkuro
Copy link
Member

ch-go for writing and clickhouse-go for reading

yes

@zzzk1
Copy link
Contributor

zzzk1 commented Jan 27, 2025

@yurishkuro Should we first introduce the config in pkg/clickhouse and create the configuration, allowing the storage/clickhouse /options to be reused similarly to how they are implemented in Elasticsearch and Kafka?

@adityachopra29
Copy link
Contributor

@yurishkuro Should we first introduce the config in pkg/clickhouse and create the configuration, allowing the storage/clickhouse /options to be reused similarly to how they are implemented in Elasticsearch and Kafka?

Yes. This makes sense. I see now that the badger storage location is outdated and also needs to refactored to be in the pkg/badger just like Cassandra, ES, and Kafka. I'll make that change into the pr.

@yurishkuro
Copy link
Member

I'm closing this, since it invites PRs that are not needed at this time. We start with implementation and integration tests, not with configuration which we have no way of validating.

@yurishkuro yurishkuro closed this as not planned Won't fix, can't repro, duplicate, stale Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants