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

make jaeger span attribute-to-tag conversion exhaustive #5574

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

arlyon
Copy link
Contributor

@arlyon arlyon commented Dec 7, 2024

Description

Right now, our quickwit span -> jaeger trace code doesn't support objects or null values and instead completely errors out which means that any trace where there is at least one span with an object with it does not work with Jaeger. I resolved this by adding support for Object and Null variants, encoding object keys delimited with ..

2024-12-07T11:42:07.655Z ERROR quickwit_serve::jaeger_api::rest_handler: failed to fetch traces error=Status { code: Internal, message: "Failed to serialize attributes: unexpected type `Object {}`", source: None }

"Failed to serialize attributes: unexpected type `{value:?}`"

I opted to use iterools::Either because it was already available and avoids heap allocation except in the recursive 'flattening' case. I believe this will not impact performance of existing, functioning, use cases at all, in fact collect is shown to be faster in some cases, though I have not benchmarked it as it is a relatively cold part of the codebase.

How was this PR tested?

Docker build. Previously traces would error, now they don't.

@CLAassistant
Copy link

CLAassistant commented Dec 7, 2024

CLA assistant check
All committers have signed the CLA.

@arlyon arlyon marked this pull request as ready for review December 10, 2024 15:40
@arlyon
Copy link
Contributor Author

arlyon commented Dec 16, 2024

Hello, any chance I can get CI approved at least? CLA signed.

Copy link
Member

@guilload guilload left a comment

Choose a reason for hiding this comment

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

Please update the test test_otlp_attributes_to_jaeger_tags accordingly.

@arlyon
Copy link
Contributor Author

arlyon commented Dec 16, 2024

Updated, thanks!

@guilload guilload merged commit 3ce58e7 into quickwit-oss:main Dec 17, 2024
4 of 5 checks passed
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