-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[clickhouse] Convert OTel traces model to native format #6935
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
[clickhouse] Convert OTel traces model to native format #6935
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6935 +/- ##
==========================================
+ Coverage 96.03% 96.11% +0.07%
==========================================
Files 355 357 +2
Lines 20993 21544 +551
==========================================
+ Hits 20161 20707 +546
- Misses 626 631 +5
Partials 206 206
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:
|
scopeVersion proto.ColStr | ||
spanAttributesKeys = new(proto.ColStr).LowCardinality().Array() | ||
spanAttributesValues = new(proto.ColStr).LowCardinality().Array() | ||
duration proto.ColUInt64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no equivalent to timestamp for duration?
timestamp = new(proto.ColDateTime64).WithPrecision(proto.PrecisionNano)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proto.ColDateTime64
is used as an accurate instant timestamp. Why change the duration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because duration and timestamp as strong types usually go together, if CH has that ability to represent duration natively I would use it. Otherwise as int64 you don't know what units those are, etc.
internal/storage/v2/clickhouse/tracestore/dbmodel/traces_test.go
Outdated
Show resolved
Hide resolved
ValueTypeEmpty = pcommon.ValueTypeEmpty | ||
ValueTypeMap = pcommon.ValueTypeMap | ||
ValueTypeSlice = pcommon.ValueTypeSlice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/README.md#attribute
It is specified that the Value cannot be nil and must only be a basic data type or its homogeneous array.
For Empty
and slice
, their values are uncertain; they could be{ }
or other basic types. Ensuring that the type is not lost seems to leave only JSON format storage as a viable solution. Map
is neither a basic data type nor a homogeneous array (it is pcommon.Map
). What should we do??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ss.Scope().Attributes().PutEmpty("empty").FromRaw("1")
ss.Scope().Attributes().PutEmptyMap("a").FromRaw(make(map[string]any))
ss.Scope().Attributes().PutEmptyBytes("bytes").FromRaw([]byte("as you can see."))
ss.Scope().Attributes().PutEmptySlice("slice").FromRaw([]any{1, 2, 3})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we always converted map and slice to JSON representation. I am not sure we need to support ValueTypeEmpty, it's ok to drop such attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClickHouse also cannot directly store []byte
. I see that ch-go
provides support for []byte
(but it actually converts it to a string for storage):
However, the []byte
here has already been Base64 encoded into a string. When using ColBytes
, we first explicitly convert the string to []byte
, but it is ultimately stored as ColStr
:
So, should we actually use ColUint8
for storage? Why not directly use Base64 string storage? After all, strings are more readable.
938515c
to
7b58c84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented support for writing complex structures in Attributes
(it works correctly in local tests). I will add unit tests later.
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go
Outdated
Show resolved
Hide resolved
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel_test.go
Outdated
Show resolved
Hide resolved
internal/storage/v2/clickhouse/tracestore/dbmodel/trace_test.go
Outdated
Show resolved
Hide resolved
|
||
func TestFromPtrace(t *testing.T) { | ||
trace := jsonToTrace(t, "ptrace.json") | ||
input := FromPtrace(trace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we usually try to do a roundtrip from-to-from, to ensure that the translation to dbmodel and back is lossless (as that's the primary requirement for storage format).
dc79552
to
4c2585e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a high level question - what is different about the implementation you're trying to write compared to otel-contrib's implementation? I assume some of the value encoding is different, can you summarize it somewhere, perhaps in a README file in the packagee.
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go
Outdated
Show resolved
Hide resolved
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go
Outdated
Show resolved
Hide resolved
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go
Outdated
Show resolved
Hide resolved
// For basic data types (such as bool, uint64, and float64), type safety is guaranteed when converting back from a string. | ||
// For non-basic types (such as Map, Slice, and []byte), they are serialized and stored as JSON-formatted strings, | ||
// ensuring that the original type is preserved when reading | ||
result[typ][k] = v.AsString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I follow. If you convert everything to strings, it means we cannot do analytics queries on the numeric attributes, which are the main reason we want to use CH. Also, isn't AsString()
a debugging feature of ptrace model? In other words it does not guarantee how the value is being returned. Nothing requires it to return a valid JSON, for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the processing only involves complex types like map
, slice
, and bytes
. For basic types, I will convert them back before writing. For example, for an int64
that was converted to a string
, I would use strconv.ParseInt(v, 10, 64)
.
If you don't think AsString()
is a good choice, another implementation would be to use map[pcommon.ValueType]map[string]pcommon.Value
instead of map[pcommon.ValueType]map[string]string
for grouping and collecting. Then, complex types would be converted to JSON, while basic types would remain unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what is the direction you are going in? I think OTLP/JSON is a good representation for complex types, but you mentioned you can't implemented over pdata.Value
without extension in OTEL? Did you open a ticket there? Or did you find an alternative solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will only convert complex types to OTLP JSON strings. For basic types, it is not necessary to first convert them to strings and then back. I create a featute request: open-telemetry/opentelemetry-collector#12826 .
I am currently trying other solutions which are quite tricky but don't depend on the OTel collector pipe data.
- Based on common.pb.go, establish the same processing logic as
pcommon.Value
. In other words, we need to implement anotherpcommon.Value
; only in this way can we generate data in OTLP/JSON format. This requires us to copy a significant amount of duplicate code. - Convert the complex types in
pcommon.Value
to our implementedpcommon.Value
type. In this way, we can directly add theMarshalValue
method, thereby obtaining the corresponding OTLP/JSON data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I can make a suggestion - put this issue on a back burner. There's lots of other work to be done to support CH, and support for these complex types is not the highest priority, it can be improved later (especially if the OTEL ticket is solved as you asked for). Aside from complex types support is this PR ready for review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw I would suggest creating a child ticket under the main CH Support issue to keep track of these deferred tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, drop complex types, introduce basic types here. And ready for review now.
Yes, i will do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if Otel will accept my proposal.
ac49a71
to
8560ef6
Compare
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go
Outdated
Show resolved
Hide resolved
resourceAttributesIntKey = new(proto.ColStr).LowCardinality().Array() | ||
resourceAttributesIntValue = new(proto.ColInt64).Array() | ||
resourceAttributesStrKey = new(proto.ColStr).LowCardinality().Array() | ||
resourceAttributesStrValue = new(proto.ColStr).LowCardinality().Array() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do we know values are low cardinality? they are user-provided values, can be anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't know, but is it common to store over 10,000 values? https://clickhouse.com/docs/sql-reference/data-types/lowcardinality#description
Also, they are very helpful when testing; otherwise, I would need to handle the byte stream myself (which I am trying to do)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go
Outdated
Show resolved
Hide resolved
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go
Outdated
Show resolved
Hide resolved
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go
Outdated
Show resolved
Hide resolved
internal/storage/v2/clickhouse/tracestore/dbmodel/from_dbmodel.go
Outdated
Show resolved
Hide resolved
return trace | ||
} | ||
|
||
func AttributesGroupToMap(span ptrace.Span, group AttributesGroup) pcommon.Map { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func AttributesGroupToMap(span ptrace.Span, group AttributesGroup) pcommon.Map { | |
func attributesGroupToMap(group AttributesGroup, warn func(string)) pcommon.Map { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we should write warn
directly into the attributes of the relevant entity (like Span Event, Resource, Scope, etc.) instead of writing it into the Span attributes?
41987b5
to
f852e0b
Compare
boolKeyCol := attrColumnsMap[ValueTypeBool].keyCol | ||
boolKeyCol.(*proto.ColArr[string]).Append(group.BoolKeys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I follow the data ownership here. AllAttributeColumns
is a static value. So are attrColumnsMap
and boolKeyCol
. What happens when we we call boolKeyCol.Append(group.BoolKeys)
where group.BoolKeys
is a dynamic value for a given trace only?
|
||
// ToDBModel Converts the OTel pipeline Traces into a ClickHouse-compatible format for batch insertion. | ||
// It maps the trace attributes, spans, links and events from the OTel model to the appropriate ClickHouse column types | ||
func ToDBModel(td ptrace.Traces) proto.Input { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move to the top of the file
If ToDBModel()
returns proto.Input
, what value does dbmodel.go provide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proto.Input
is the compatible format for ClickHouse batch insertion. The conversion path can be ptrace -> dbmodel.Trace -> proto.Input. Considering the final target is proto.Input, converting to the intermediate structure dbmodel.Trace introduce unnecessary complexity. Why not convert directly from ptrace to proto.Input
? Furthermore, dbmodel.go
is also used for handling read operations (with clickhouse-go), responsible for mapping ClickHouse table fields to corresponding Go structs.
internal/storage/v2/clickhouse/tracestore/dbmodel/to_dbmodel.go
Outdated
Show resolved
Hide resolved
…r batch insertion Signed-off-by: zhengkezhou1 <[email protected]>
Signed-off-by: zhengkezhou1 <[email protected]>
Signed-off-by: zhengkezhou1 <[email protected]>
Signed-off-by: zhengkezhou1 <[email protected]>
Signed-off-by: zhengkezhou1 <[email protected]>
Signed-off-by: zhengkezhou1 <[email protected]>
Signed-off-by: zhengkezhou1 <[email protected]>
1976d7a
to
b885c23
Compare
timestampCol := traceColumnSet.span.timestamp.Col | ||
timestampCol.(*proto.ColDateTime64).Append(span.StartTimestamp().AsTime()) | ||
traceIDCol := traceColumnSet.span.traceID.Col | ||
traceIDCol.(*proto.ColLowCardinality[string]).Append(traceIDToHexString(span.TraceID())) | ||
spanIDCol := traceColumnSet.span.spanID.Col | ||
spanIDCol.(*proto.ColLowCardinality[string]).Append(spanIDToHexString(span.SpanID())) | ||
parentSpanIDCol := traceColumnSet.span.parentSpanID.Col | ||
parentSpanIDCol.(*proto.ColLowCardinality[string]).Append(spanIDToHexString(span.ParentSpanID())) | ||
traceStateCol := traceColumnSet.span.traceState.Col | ||
traceStateCol.(*proto.ColLowCardinality[string]).Append(span.TraceState().AsRaw()) | ||
spanNameCol := traceColumnSet.span.name.Col | ||
spanNameCol.(*proto.ColLowCardinality[string]).Append(span.Name()) | ||
spanKindCol := traceColumnSet.span.kind.Col | ||
spanKindCol.(*proto.ColLowCardinality[string]).Append(span.Kind().String()) | ||
scopeNameCol := traceColumnSet.scope.name.Col | ||
scopeNameCol.(*proto.ColLowCardinality[string]).Append(scope.Name()) | ||
scopeVersion := traceColumnSet.scope.version.Col | ||
scopeVersion.(*proto.ColLowCardinality[string]).Append(scope.Version()) | ||
durationCol := traceColumnSet.span.duration.Col | ||
durationCol.(*proto.ColDateTime64).Append(span.EndTimestamp().AsTime()) | ||
statusCodeCol := traceColumnSet.span.statusCode.Col | ||
statusCodeCol.(*proto.ColLowCardinality[string]).Append(span.Status().Code().String()) | ||
statusMessageCol := traceColumnSet.span.statusMessage.Col | ||
statusMessageCol.(*proto.ColLowCardinality[string]).Append(span.Status().Message()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that not all fields should be LowCardinality. This is because the difference between the String type and LowCardinality(String) during writing makes testing and comparison very difficult, as I mentioned before.
|
||
resourceAttributes, err := AttributesGroupToMap(dbTrace.Resource.Attributes) | ||
if err != nil { | ||
jptrace.AddWarnings(span, fmt.Sprintf("failed to decode bytes value: %v", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there's no indication at this point in the code that the error is about "failed to decode bytes". Such error can be returned near the code that actually attempted decoding byte values. The most we can say here is "failed to decode attributes: %w"
scope := scopeSpans.Scope() | ||
sc, err := FromDBScope(dbTrace.Scope) | ||
if err != nil { | ||
jptrace.AddWarnings(span, fmt.Sprintf("failed to decode bytes value: %v", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue with error message
span.SetStartTimestamp(pcommon.NewTimestampFromTime(s.Timestamp)) | ||
traceId, err := hex.DecodeString(s.TraceId) | ||
if err != nil { | ||
return span, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"failed to decode trace ID: %w"
"github.com/jaegertracing/jaeger/internal/jptrace" | ||
) | ||
|
||
func FromDBModel(dbTrace Trace) ptrace.Traces { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would dbTrace object be created before this function is called? Do you have a pointer in the old (full) PR as example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: zhengkezhou1 <[email protected]>
…g#6935) ## Which problem is this PR solving? - Part of jaegertracing#5058 ## Description of the changes - Based on the `ch-go` wire protocol, convert the OTel traces model to the ClickHouse native format for batch insertion. ## How was this change tested? - unit tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: zhengkezhou1 <[email protected]>
…g#6935) ## Which problem is this PR solving? - Part of jaegertracing#5058 ## Description of the changes - Based on the `ch-go` wire protocol, convert the OTel traces model to the ClickHouse native format for batch insertion. ## How was this change tested? - unit tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: zhengkezhou1 <[email protected]> Signed-off-by: amol-verma-allen <[email protected]>
Which problem is this PR solving?
Description of the changes
ch-go
wire protocol, convert the OTel traces model to the ClickHouse native format for batch insertion.How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test