Fix traceid validator#879
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@googlebot I fixed it. |
|
@aabmass @hekike @draffensperger @mayurkale22 can you say sth about the status of this project wrt PRs like this and also security updates ? (see my previous comment) |
|
Hey @philicious, we are still accepting bugfixes. Sorry for not taking a look at this PR earlier. You are right also about security fixes, I need to merge these in but it is not trivial because some of the security fixes require dropping node8 and extensive code changes. |
aabmass
left a comment
There was a problem hiding this comment.
Thanks for the PR! Just a few comments
|
thanks @aabmass for approval and also for working on security updates ! 👍 PS: as I am not authorized for merging, you will have to do so |
|
@philicious can you just rebase/merge? I can merge the PR after that 😄 |
done ✅ @aabmass , thank you ! PS: we are looking forward to a new opencensus-node release, including this PR and some security fixes 👍 😄 💯 |
|
@aabmass friendly reminder :) |
|
@aabmass what is the status of the opencensus project? to us it looks like it has no active maintainers anymore and we, as an enterprise company, have to consider using a hard or private fork to ensure security and bugfixes are done if you need help maintaining, we would potentially be interested |
|
@draffensperger @mayurkale22 @hekike could you pls have a look at this PR ? |
|
@philicious apologies for the delay on this. We'll get back to you shortly. |
aabmass
left a comment
There was a problem hiding this comment.
@philicious so sorry for the delay. We plan to keep maintaining this, but it's just me at the moment.
if you need help maintaining, we would potentially be interested
That would be awesome, please let me know if you have bandwidth to help!
Fix inject validators Fix extract formatter for leading zero bugs
oh, that sounds like very few maintainers for such a huge project ! I'll discuss this opportunity with my colleagues, so we can at least ensure security fixes, while OpenTelemetry is not fully matured and people (like us) rely on OpenCensus for having distributed tracing |
Sounds good, please open an issue and tag me when you find out more. Thanks for the PR! |
jaeger format traceId validator seems to have been copied from the W3C TraceContext format validator as the function comments suggest.
however there is a difference as jaeger also allows shorter (64-bit) traceIds, while W3C doesnt.
this leads to unexpected behavior. e.g. if you start the trace with nginx-ingress, which creates 64-bit traceId. If those are propagated, the subsequent service will fail the validation on injection when doing a http call to yet another service. ultimately, the subsequent services are shown as children of nginx and not as children of the other service.
its working as expected if you bypass nginx and the initial traceId is 128bit length