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

Create Clickhouse config #6622

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

adityachopra29
Copy link
Contributor

Which problem is this PR solving?

Description of the changes

  • Create plugin/storage/clickhouse/
  • This is a draft pr, not ready to be merged.

How was this change tested?

Checklist

Signed-off-by: adityachopra29 <[email protected]>
Signed-off-by: adityachopra29 <[email protected]>
Signed-off-by: adityachopra29 <[email protected]>
Signed-off-by: adityachopra29 <[email protected]>
@adityachopra29 adityachopra29 requested a review from a team as a code owner January 27, 2025 17:29
@dosubot dosubot bot added the area/storage label Jan 27, 2025
@adityachopra29
Copy link
Contributor Author

adityachopra29 commented Jan 27, 2025

This is a draft pr.
@yurishkuro I am having trouble writing the schema config, as we dont have a schema ready right now.
Also wanted to confirm whether I am on the right track.
Also, wanted to understand how we will be deciding the default configuration values.
I took inspiration from the badger and cassandra config and structure

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 95.97%. Comparing base (20bdd8d) to head (1536983).

Files with missing lines Patch % Lines
pkg/clickhouse/config/config.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6622      +/-   ##
==========================================
- Coverage   96.00%   95.97%   -0.04%     
==========================================
  Files         365      366       +1     
  Lines       20616    20623       +7     
==========================================
  Hits        19792    19792              
- Misses        626      633       +7     
  Partials      198      198              
Flag Coverage Δ
badger_v1 9.92% <ø> (ø)
badger_v2 1.84% <ø> (ø)
cassandra-4.x-v1-manual 15.08% <ø> (ø)
cassandra-4.x-v2-auto 1.83% <ø> (ø)
cassandra-4.x-v2-manual 1.83% <ø> (ø)
cassandra-5.x-v1-manual 15.08% <ø> (ø)
cassandra-5.x-v2-auto 1.83% <ø> (ø)
cassandra-5.x-v2-manual 1.83% <ø> (ø)
elasticsearch-6.x-v1 19.30% <ø> (ø)
elasticsearch-7.x-v1 19.38% <ø> (ø)
elasticsearch-8.x-v1 19.56% <ø> (ø)
elasticsearch-8.x-v2 1.84% <ø> (-0.12%) ⬇️
grpc_v1 11.04% <ø> (ø)
grpc_v2 7.89% <ø> (ø)
kafka-3.x-v1 10.21% <ø> (ø)
kafka-3.x-v2 1.84% <ø> (ø)
memory_v2 1.84% <ø> (ø)
opensearch-1.x-v1 19.44% <ø> (ø)
opensearch-2.x-v1 19.44% <ø> (ø)
opensearch-2.x-v2 1.84% <ø> (ø)
tailsampling-processor 0.48% <ø> (ø)
unittests 94.78% <0.00%> (-0.04%) ⬇️

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.

@adityachopra29 adityachopra29 marked this pull request as draft January 27, 2025 17:44
@adityachopra29
Copy link
Contributor Author

Just realised a structure change is required. Will make it and update the pr shortly.

@yurishkuro
Copy link
Member

What is the motivation for the specific config parameters you are introducing? It feels a bit like putting the horse in front of the carriage - we don't have an implementation of the storage yet, so how do we know which config options will even be required?

Copy link
Contributor

@zzzk1 zzzk1 left a comment

Choose a reason for hiding this comment

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

Comment on lines 82 to 85
type BasicAuthenticator struct {
Username string `mapstructure:"username"`
Password string `mapstructure:"password"`
}
Copy link
Contributor

@zzzk1 zzzk1 Jan 27, 2025

Choose a reason for hiding this comment

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

Only the following are required for a mini connection: 1. host 2. port 3. database 4. username 5. password.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@adityachopra29 adityachopra29 marked this pull request as ready for review January 28, 2025 17:56
@adityachopra29
Copy link
Contributor Author

adityachopra29 commented Jan 28, 2025

What is the motivation for the specific config parameters you are introducing? It feels a bit like putting the horse in front of the carriage - we don't have an implementation of the storage yet, so how do we know which config options will even be required?

@yurishkuro I see. I looked through the configurations for the other db storages, and saw what types of configurations jaeger was using to model config for clickhouse. But adding config as we develop is more logical.
I have moved the config to /pkg/clickhouse and also removed the extra config. Pls have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants