-
Notifications
You must be signed in to change notification settings - Fork 353
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
refactor: update jaeger api implementation for new trace modeling #5655
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
The idea of this patch is to support both v0 and v1 data model in single query without adding additional check. It works for most cases except when the query contains tag filter, it's a little bit tricky to build the filter. |
thinking about this for a while and there is no easy approach to use single statement for tag filters on two table models. I will need to add table attribute to mark trace table version model |
Is it possible to add table comments stating whether it's using v0 or v1? Thus, we can distinguish between the two models. |
@shuiyisong yes I'm working on similar mechanism to add this information in table options |
The refactor is done, please take a look @zyy17 @shuiyisong |
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.
mostly lgtm
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#5595
What's changed and what's your intention?
This patch updates how we assemble jaeger API response in order to fit both legacy trace modeling and the upcoming newer one
greptime_trace_v1
.changes included in this patch are:
TABLE_DATA_MODEL
to mark special table models we built into database by ourselvestable
v0
andv1
data model for jaeger API. This is done by*
to retrieve all columns in trace/span queryspan_attributes
column, it'sv0
and we will assemble the span tags as json. Otherwise it'sv1
model and we will push all columns start withspan_attributes.
into span tags.order by timestamp
by defaultPR Checklist
Please convert it to a draft if some of the following conditions are not met.