Migrate from ch-go to clickhouse-go library #7552
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Somil Jain <[email protected]>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
59
to
63
| data, err := json.Marshal(convertValueToInterface(v)) | ||
| if err == nil { | ||
| group.StrKeys = append(group.StrKeys, k) | ||
| group.StrValues = append(group.StrValues, string(data)) | ||
| } |
There was a problem hiding this comment.
The JSON marshaling error is silently ignored, causing the attribute to be dropped when serialization fails. Consider either logging the error or implementing a fallback serialization method to ensure all attribute data is preserved. This is particularly important for observability data where losing attributes could impact troubleshooting capabilities.
Suggested change
| data, err := json.Marshal(convertValueToInterface(v)) | |
| if err == nil { | |
| group.StrKeys = append(group.StrKeys, k) | |
| group.StrValues = append(group.StrValues, string(data)) | |
| } | |
| data, err := json.Marshal(convertValueToInterface(v)) | |
| if err != nil { | |
| // Log the error | |
| log.Printf("Failed to marshal attribute %s: %v, using fallback serialization", k, err) | |
| // Fallback: use string representation | |
| fallbackValue := fmt.Sprintf("%v", v) | |
| data, _ = json.Marshal(fallbackValue) | |
| } | |
| group.StrKeys = append(group.StrKeys, k) | |
| group.StrValues = append(group.StrValues, string(data)) |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7552 +/- ##
==========================================
- Coverage 96.51% 96.44% -0.07%
==========================================
Files 385 385
Lines 23364 23116 -248
==========================================
- Hits 22549 22294 -255
- Misses 627 633 +6
- Partials 188 189 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Metrics Comparison SummaryTotal changes across all snapshots: 53 Detailed changes per snapshotsummary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 53
🆕 Added Metrics
View diff sample+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="+Inf",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="0",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="100",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="1000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="25",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
...View diff sample+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="+Inf",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.005",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.01",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.025",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.05",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.075",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.1",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
...View diff sample+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="+Inf",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="0",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="100",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="1000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="25",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
... |
Signed-off-by: Somil Jain <[email protected]>
Signed-off-by: Somil Jain <[email protected]>
Comment on lines
57
to
65
| case ValueTypeSlice, ValueTypeMap: | ||
| // For complex types, serialize to JSON string | ||
| data, err := json.Marshal(convertValueToInterface(v)) | ||
| if err != nil { | ||
| // Fallback: use string representation if JSON marshaling fails | ||
| data, _ = json.Marshal(v.AsString()) | ||
| } | ||
| group.StrKeys = append(group.StrKeys, k) | ||
| group.StrValues = append(group.StrValues, string(data)) |
There was a problem hiding this comment.
There appears to be a schema mismatch between how complex types are stored and how they're referenced in the SQL query. The code is serializing ValueTypeSlice and ValueTypeMap to JSON strings and adding them to StrKeys and StrValues, but the SQL query in queries.go expects these values in complex_attributes.key and complex_attributes.value columns. This inconsistency will likely cause data to be inserted into incorrect columns or query failures when the column count doesn't match parameter count.
Consider either:
- Adding the serialized complex types to
BytesKeysandBytesValues(which appear to be used for complex attributes based on the SQL schema), or - Updating the SQL query to match the current implementation where complex types are stored in string columns
Suggested change
| case ValueTypeSlice, ValueTypeMap: | |
| // For complex types, serialize to JSON string | |
| data, err := json.Marshal(convertValueToInterface(v)) | |
| if err != nil { | |
| // Fallback: use string representation if JSON marshaling fails | |
| data, _ = json.Marshal(v.AsString()) | |
| } | |
| group.StrKeys = append(group.StrKeys, k) | |
| group.StrValues = append(group.StrValues, string(data)) | |
| case ValueTypeSlice, ValueTypeMap: | |
| // For complex types, serialize to JSON string | |
| data, err := json.Marshal(convertValueToInterface(v)) | |
| if err != nil { | |
| // Fallback: use string representation if JSON marshaling fails | |
| data, _ = json.Marshal(v.AsString()) | |
| } | |
| group.BytesKeys = append(group.BytesKeys, k) | |
| group.BytesValues = append(group.BytesValues, data) |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Comment on lines
+78
to
+83
| var linkTraceIDs, linkSpanIDs, linkTraceStates []string | ||
| for _, link := range span.Links().All() { | ||
| linkTraceIDs = append(linkTraceIDs, link.TraceID().String()) | ||
| linkSpanIDs = append(linkSpanIDs, link.SpanID().String()) | ||
| linkTraceStates = append(linkTraceStates, link.TraceState().AsRaw()) | ||
| } |
There was a problem hiding this comment.
The code extracts link metadata (trace IDs, span IDs, trace states) but doesn't capture link attributes, which appears inconsistent with how event attributes are handled. This creates a potential data loss situation where link context is only partially preserved. Consider implementing link attribute extraction using a similar pattern to the event attributes code above:
// Extract link attributes
var linkBoolKeys, linkDoubleKeys, linkIntKeys, linkStrKeys, linkComplexKeys [][]string
var linkBoolVals [][]bool
var linkDoubleVals [][]float64
var linkIntVals [][]int64
var linkStrVals, linkComplexVals [][]string
for _, link := range span.Links().All() {
// existing code for IDs and states
linkAttrs := dbmodel.ExtractAttributes(link.Attributes())
// populate attribute arrays similar to events
}This would ensure complete data preservation across all span components.
Suggested change
| var linkTraceIDs, linkSpanIDs, linkTraceStates []string | |
| for _, link := range span.Links().All() { | |
| linkTraceIDs = append(linkTraceIDs, link.TraceID().String()) | |
| linkSpanIDs = append(linkSpanIDs, link.SpanID().String()) | |
| linkTraceStates = append(linkTraceStates, link.TraceState().AsRaw()) | |
| } | |
| var linkTraceIDs, linkSpanIDs, linkTraceStates []string | |
| var linkBoolKeys, linkDoubleKeys, linkIntKeys, linkStrKeys, linkComplexKeys [][]string | |
| var linkBoolVals [][]bool | |
| var linkDoubleVals [][]float64 | |
| var linkIntVals [][]int64 | |
| var linkStrVals, linkComplexVals [][]string | |
| for _, link := range span.Links().All() { | |
| linkTraceIDs = append(linkTraceIDs, link.TraceID().String()) | |
| linkSpanIDs = append(linkSpanIDs, link.SpanID().String()) | |
| linkTraceStates = append(linkTraceStates, link.TraceState().AsRaw()) | |
| linkAttrs := dbmodel.ExtractAttributes(link.Attributes()) | |
| linkBoolKeys = append(linkBoolKeys, linkAttrs.BoolKeys) | |
| linkBoolVals = append(linkBoolVals, linkAttrs.BoolValues) | |
| linkDoubleKeys = append(linkDoubleKeys, linkAttrs.DoubleKeys) | |
| linkDoubleVals = append(linkDoubleVals, linkAttrs.DoubleValues) | |
| linkIntKeys = append(linkIntKeys, linkAttrs.IntKeys) | |
| linkIntVals = append(linkIntVals, linkAttrs.IntValues) | |
| linkStrKeys = append(linkStrKeys, linkAttrs.StringKeys) | |
| linkStrVals = append(linkStrVals, linkAttrs.StringValues) | |
| linkComplexKeys = append(linkComplexKeys, linkAttrs.ComplexKeys) | |
| linkComplexVals = append(linkComplexVals, linkAttrs.ComplexValues) | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Signed-off-by: Somil Jain <[email protected]>
SomilJain0112
force-pushed
the
feat/Migrate-ch-go-library-to-clickhouse
branch
from
fce4725 to
1b85e41
Compare
|
@mahadzaryab1 do we need this? |
E2E tests were failing because batch.Send() wasn't properly committing data. Adding explicit Flush() before Send() ensures all pending data is flushed to ClickHouse before the batch is sent. Changes: - writer.go: Add batch.Flush() before batch.Send() - driver_test.go: Add Flush() method to testBatch mock This should fix the E2E test failures where 0 spans were being retrieved after writing. Signed-off-by: Somil Jain <[email protected]>
Resolved merge conflicts in: - internal/storage/v2/clickhouse/sql/queries.go * Removed duplicate attribute fields in INSERT statement * Kept 38 placeholders for full schema implementation - internal/storage/v2/clickhouse/tracestore/writer.go * Kept our full implementation with dbmodel.ExtractAttributes * Preserved attribute extraction for events and links * Added missing jptrace import - internal/storage/v2/clickhouse/tracestore/writer_test.go * Kept 38-field test assertions * Service name at index 35, scope at 36, version at 37 All unit tests passing after merge. Signed-off-by: Somil Jain <[email protected]>
SomilJain0112
force-pushed
the
feat/Migrate-ch-go-library-to-clickhouse
branch
from
8a4cae8 to
45c27f8
Compare
|
Hii @mahadzaryab1 , Please let me know if this issue still needs to be resolved as I am working on it. |
Signed-off-by: Somil Jain <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
#7148
Description of the changes
How was this change tested?
Tested locally
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test