Skip to content
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

only return root spans for jaeger http api #5358

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

trinity-1686a
Copy link
Contributor

Description

on jaeger http api, return only root span. On grpc api, return all, otherwise jaeger shows empty traces until you reload the page
Can be improved once #5308 is merged, to stay mostly backward compatible (right now, the http api errors out, if otel-traces-v0_7 exists)

How was this PR tested?

tested manually with jaeger, and grafana jaeger datasource pointed to quickwit

@trinity-1686a trinity-1686a requested a review from fmassot August 28, 2024 14:21
Copy link

github-actions bot commented Aug 28, 2024

On SSD:

Average search latency is 0.998x that of the reference (lower is better).
Ref run id: 3090, ref commit: eb5e26c
Link

On GCS:

Average search latency is 1.0x that of the reference (lower is better).
Ref run id: 3091, ref commit: eb5e26c
Link

@rdettai
Copy link
Collaborator

rdettai commented Aug 28, 2024

might be of interest to @Sh4d1 (see #5122)

@@ -353,7 +354,7 @@ mod tests {
async fn test_when_jaeger_not_found() {
let jaeger_api_handler = jaeger_api_handlers(None).recover(crate::rest::recover_fn_final);
let resp = warp::test::request()
.path("/otel-traces-v0_7/jaeger/api/services")
.path("/otel-traces-v0_9/jaeger/api/services")
Copy link
Collaborator

@rdettai rdettai Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already fixed in 7184ccd

@rdettai
Copy link
Collaborator

rdettai commented Aug 29, 2024

@trinity-1686a quick thought regarding your bump of OTEL_TRACES_INDEX_ID to otel-traces-v0_9. Now that mappings can be updated, should we not set the the index id to otel-traces ?

@trinity-1686a
Copy link
Contributor Author

some people are modifying their otel-traces-v0_* indexes. We'd need some way to make sure we don't clobber their changes. That's something we should do in the future though.

@trinity-1686a trinity-1686a merged commit 67091c5 into main Aug 29, 2024
5 checks passed
@trinity-1686a trinity-1686a deleted the trinity/jaeger-return-root branch August 29, 2024 15:20
fmassot pushed a commit that referenced this pull request Sep 2, 2024
* only return root spans for jaeger http api

* fix rest api tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants