-
Notifications
You must be signed in to change notification settings - Fork 348
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
base: main
Are you sure you want to change the base?
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
|
trace_id_to_processes.entry(trace_id).or_default(); | ||
} | ||
if let JsonValue::Object(span_attrs) = cell { | ||
span.tags = object_to_tags(span_attrs); |
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.
Let's use span.tags.extend()
in case if the order is not ensured.
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.
There is only one span_attributes
in the legacy data model
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 |
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
.PR Checklist
Please convert it to a draft if some of the following conditions are not met.