[refactor] Rework ClickHouse schema structure#7181
[refactor] Rework ClickHouse schema structure#7181yurishkuro merged 11 commits intojaegertracing:mainfrom
Conversation
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1
added
the
changelog:experimental
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7181 +/- ##
==========================================
- Coverage 96.21% 96.16% -0.06%
==========================================
Files 365 366 +1
Lines 21964 21947 -17
==========================================
- Hits 21133 21105 -28
- Misses 621 629 +8
- Partials 210 213 +3
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:
|
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1
left a comment
mahadzaryab1
left a comment
There was a problem hiding this comment.
couple of questions
| type AttributesGroup struct { | ||
| BoolKeys []string | ||
| BoolValues []bool | ||
| DoubleKeys []string | ||
| DoubleValues []float64 | ||
| IntKeys []string | ||
| IntValues []int64 | ||
| StrKeys []string | ||
| StrValues []string | ||
| BytesKeys []string | ||
| BytesValues []string | ||
| } |
There was a problem hiding this comment.
@yurishkuro I don't believe this approach for attributes is compatible with ClickHouse. If we want to maintain typed attributes, I think using the JSON type might be the best approach here. Thoughts?
There was a problem hiding this comment.
Why don't you think it's not compatible? We discussed this on the original PR, please check how it was proposed to be used.
It's using the NESTED construct, which stores the attributes as sub-columnar structures. I don't know how exactly JSON support is implemented in CH but I highly doubt it would match in efficiency with explicitly managed columnar layout. Does CH even support schema for JSON fields? Without schema it would have to treat it as a full text index (similar to ES).
There was a problem hiding this comment.
@yurishkuro Ah okay sounds good! I reverted this change here to keep that struct for now. But the semantics of the conversion will need to change to follow https://clickhouse.com/docs/integrations/go#nested. I'll leave it out of the refactored model for now and bring it back cleanly in the following PRs.
| StatusCode string | ||
| StatusMessage string | ||
| Attributes AttributesGroup | ||
| // --- Span --- |
There was a problem hiding this comment.
@yurishkuro There's a couple of fields missing from the OTLP type. Do we want to store these?
- Flags
- DroppedAttributesCount
- DroppedEventsCount
- DroppedLinksCount
There was a problem hiding this comment.
yes we can add these too, doesn't hurt.
There was a problem hiding this comment.
This PR is already quite large so I'll add these in a following PR
| } | ||
| resourceAttributes.CopyTo(resource.Attributes()) | ||
| // TODO: populate attributes | ||
| // TODO: do we populate the service name from the span? |
There was a problem hiding this comment.
@yurishkuro Question for you. My guess was no and that this would already exist in the attributes we store but just wanted to double check.
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
internal/storage/v2/clickhouse/tracestore/dbmodel/from_dbmodel_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
2 participants
Which problem is this PR solving?
Description of the changes
clickhouse-godriver. It removes the nesting of types and instead stores all the fields under one struct to make it reflective of the actual schema.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test