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

chore: do not convert ids to base64 #1583

Closed
wants to merge 1 commit into from

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Oct 8, 2020

This fixes #1513

@obecny the gRPC export is still incorrect but I don't understand the gRPC/proto pipeline to understand how to fix it.

@obecny
Copy link
Member

obecny commented Oct 8, 2020

@dyladan I'm in a process of upgrading to latest proto v.0.5.0 and Im' already modifying the similar piece of code. I'm affraid they might be merging conflicts. Not sure how urgent is this fix as in fact the fix is required for next proto, and our collector exporter is not compatible with next version anyway (talking about metrics mainly, for tracing I think this is the only change), but that means that if you want to upgrade you wont be able to use metrics and traces in newest version. So upgrading might not make sense in such case, unless you only use trace.

@dyladan
Copy link
Member Author

dyladan commented Oct 9, 2020

@dyladan I'm in a process of upgrading to latest proto v.0.5.0 and Im' already modifying the similar piece of code. I'm affraid they might be merging conflicts. Not sure how urgent is this fix as in fact the fix is required for next proto, and our collector exporter is not compatible with next version anyway (talking about metrics mainly, for tracing I think this is the only change), but that means that if you want to upgrade you wont be able to use metrics and traces in newest version. So upgrading might not make sense in such case, unless you only use trace.

So you're saying we should wait for the proto 0.5.0 upgrade before doing this? That's fine if that's what you want to do.

@obecny
Copy link
Member

obecny commented Oct 9, 2020

@dyladan just checked about removing conversion on top of my PR -> #1588.
Besides what you changed the missing part for grpc was to upgrade also the ids for grpc. Whatever suits you better, I can either push it to my existing PR, create a new PR to be merged to my PR, or you can add missing part of ids for grpc into your PR (I haven't yet pushed the changes to my PR)
WDYT ?

@dyladan
Copy link
Member Author

dyladan commented Oct 9, 2020

Do you mean just update the ids in the tests?

@dyladan
Copy link
Member Author

dyladan commented Oct 14, 2020

@obecny did you do this work in #1588?

@obecny
Copy link
Member

obecny commented Oct 14, 2020

@obecny did you do this work in #1588?

Yes, after @vmarchaud tests it was the last thing that was still missing for proto v.0.5.0

@dyladan dyladan closed this Oct 14, 2020
@dyladan
Copy link
Member Author

dyladan commented Oct 14, 2020

ok then i'm closing this

@dyladan dyladan deleted the hex-ids branch October 14, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use hex encoding for trace id and span id fields in OTLP JSON encoding
2 participants