-
Notifications
You must be signed in to change notification settings - Fork 498
Jaegerv2 #6023
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
base: main
Are you sure you want to change the base?
Jaegerv2 #6023
Conversation
|
|
||
| if root_only { | ||
| let is_root = UserInputQuery { | ||
| user_text: "NOT is_root:false".to_string(), |
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.
why not is_root:true ? Why not a term query? Is this in v1?
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/quickwit-oss/quickwit/blob/main/quickwit/quickwit-jaeger/src/lib.rs#L322 same in v1 implementation
I don't think a term query is appropriate because, if I understand correctly, a term query searches for fields where is_root is false, or in cases where there are child spans, the field does not exist
| default_operator: BooleanOperand::And, | ||
| lenient: true, | ||
| }; | ||
| let mut new_query = BoolQuery::default(); |
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.
nitpick: BooleanQuery::intersection
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 can't find the BoolQuery::intersection method, are you sure it exists?
fulmicoton
left a comment
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.
A couple of nitpicks here and there.
Description
Jaeger has a new specification for the GRPC remote storage API. They have removed the old version from release 2.6 onwards
https://github.com/jaegertracing/jaeger/tree/v2.13.0/internal/storage/v2/grpc
How was this PR tested?
I add unit test for grpc endpoint and new helper function. And I tested the features with the latest version of Jaeger, and everything works fine.