Skip to content

[clickhouse]: deprecated the dbmodel base on ch-go library #7149

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
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[clickhouse]: deprecated the dbmodel base on ch-go library #7149

wants to merge 4 commits into from

Conversation

zhengkezhou1
Copy link
Contributor

Which problem is this PR solving?

Description of the changes

  • Deprecated exist db model base on ch-go and functions:to_dbmodel from_dbmodel.
  • Introduce new db model and base on the clickhouse-go only.

How was this change tested?

  • unit tests.

Checklist

@zhengkezhou1 zhengkezhou1 requested a review from a team as a code owner May 20, 2025 03:43
@zhengkezhou1 zhengkezhou1 requested a review from jkowall May 20, 2025 03:44
@zhengkezhou1 zhengkezhou1 mentioned this pull request May 20, 2025
Open
4 tasks
@codecov Codecov
Copy link

codecov bot commented May 20, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 98.16514% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.15%. Comparing base (0d02bc8) to head (460c037).
⚠️ Report is 235 commits behind head on main.

Files with missing lines Patch % Lines
...age/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go 97.29% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7149      +/-   ##
==========================================
- Coverage   96.21%   96.15%   -0.06%     
==========================================
  Files         361      361              
  Lines       21779    21483     -296     
==========================================
- Hits        20955    20658     -297     
  Misses        616      616              
- Partials      208      209       +1
Flag Coverage Δ
badger_v1 9.87% <ø> (ø)
badger_v2 1.90% <ø> (ø)
cassandra-4.x-v1-manual 14.85% <ø> (ø)
cassandra-4.x-v2-auto 1.89% <ø> (ø)
cassandra-4.x-v2-manual 1.89% <ø> (ø)
cassandra-5.x-v1-manual 14.85% <ø> (ø)
cassandra-5.x-v2-auto 1.89% <ø> (ø)
cassandra-5.x-v2-manual 1.89% <ø> (ø)
elasticsearch-6.x-v1 20.69% <ø> (ø)
elasticsearch-7.x-v1 20.77% <ø> (ø)
elasticsearch-8.x-v1 20.95% <ø> (ø)
elasticsearch-8.x-v2 1.90% <ø> (ø)
grpc_v1 11.41% <ø> (ø)
grpc_v2 1.90% <ø> (ø)
kafka-3.x-v1 10.14% <ø> (ø)
kafka-3.x-v2 1.90% <ø> (ø)
memory_v2 1.90% <ø> (ø)
opensearch-1.x-v1 20.82% <ø> (ø)
opensearch-2.x-v1 20.82% <ø> (ø)
opensearch-2.x-v2 1.90% <ø> (ø)
query 1.90% <ø> (ø)
tailsampling-processor 0.51% <ø> (ø)
unittests 94.95% <98.16%> (-0.08%) ⬇️

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.

@mahadzaryab1
Copy link
Collaborator

@zhengkezhou1 Since all of the code is new and not used elsewhere, can we make the required changes directly in-place?

@mahadzaryab1 mahadzaryab1 self-requested a review May 20, 2025 12:20
@zhengkezhou1
update
caef797 
Signed-off-by: zhengkezhou1 <[email protected]>
@zhengkezhou1
Copy link
Contributor Author

I make the changes directly.

@mahadzaryab1 mahadzaryab1 added the changelog:exprimental Change to an experimental part of the code label May 21, 2025
mahadzaryab1
mahadzaryab1 reviewed May 21, 2025
View reviewed changes
internal/storage/v2/clickhouse/tracestore/dbmodel/from_dbmodel.go
)

// FromDBModel convert the Trace to ptrace.Traces
func FromDBModel(dbTrace Trace) ptrace.Traces {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having this function be static - is it possible to have it be received by dbmodel.Trace? Is there any reason we cannot do that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

and same for the other types? maybe we could move them each to their own file to keep the files lightweight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and same for the other types? maybe we could move them each to their own file to keep the files lightweight?

What do you mean by moving those to different files?
Is that use a method instead of a function and move it into dbmodel.go?

type Link struct {
	TraceId    []byte
	SpanId     []byte
	TraceState string
	Attributes AttributesGroup
}

func (link Link) fromLink() ptrace.SpanLink {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of having this function be static - is it possible to have it be received by dbmodel.Trace? Is there any reason we cannot do that?

It's easy to test and All these functions (FromDBModel, fromDBScope, fromDBSpan, fromDBEvent, fromDBLink, fromDBSpanKind, fromDBStatusCode, attributesGroupToMap) are purely converting from one data structure (dbmodel) to another (ptrace or pcommon). These conversion functions don't rely on any external state and don't modify the state of the objects being converted. They are side-effect-free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mahadzaryab1 How do you think about this?

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 4, 2025

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 github-actions bot added stale The issue/PR has become stale and may be auto-closed and removed stale The issue/PR has become stale and may be auto-closed labels Aug 4, 2025
@github-actions GitHub Actions
Copy link

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 github-actions bot added the stale The issue/PR has become stale and may be auto-closed label Oct 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mahadzaryab1 mahadzaryab1 mahadzaryab1 left review comments

@jkowall jkowall Awaiting requested review from jkowall jkowall 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 changelog:exprimental Change to an experimental part of the code stale The issue/PR has become stale and may be auto-closed v2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[clickhouse]: deprecated the use of the ch-go library in write path

2 participants

@zhengkezhou1 @mahadzaryab1