Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Fix traceid validator #879

Merged
merged 2 commits into from
May 18, 2021
Merged

Fix traceid validator #879

merged 2 commits into from
May 18, 2021

Conversation

philicious
Copy link
Contributor

@philicious philicious commented Dec 24, 2020

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

@google-cla
Copy link

google-cla bot commented Dec 24, 2020

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.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@julianbei
Copy link

@googlebot I fixed it.

@philicious philicious marked this pull request as ready for review December 25, 2020 15:46
@philicious
Copy link
Contributor Author

@aabmass @hekike are you still accepting bugfix PRs on opencensus despite the OpenTelemetry merger going on ?

I am wondering as the open PRs suggest that even security fixes by Snyk are not merged anymore since June 🤔 despite the README saying that security fixes will be done for even 2 more yrs

@philicious
Copy link
Contributor Author

@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)

@aabmass
Copy link
Member

aabmass commented Jan 8, 2021

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.

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Just a few comments

@philicious philicious requested a review from aabmass January 27, 2021 10:21
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@philicious
Copy link
Contributor Author

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

@aabmass
Copy link
Member

aabmass commented Feb 23, 2021

@philicious can you just rebase/merge? I can merge the PR after that 😄

@philicious
Copy link
Contributor Author

@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 👍 😄 💯

@philicious
Copy link
Contributor Author

@aabmass friendly reminder :)

@philicious
Copy link
Contributor Author

@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

@philicious
Copy link
Contributor Author

@draffensperger @mayurkale22 @hekike could you pls have a look at this PR ?

@punya
Copy link

punya commented May 11, 2021

@philicious apologies for the delay on this. We'll get back to you shortly.

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

@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!

packages/opencensus-propagation-jaeger/src/validators.ts Outdated Show resolved Hide resolved
Fix inject validators
Fix extract formatter for leading zero bugs
@philicious
Copy link
Contributor Author

@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!

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

@aabmass aabmass merged commit 2ee9c92 into census-instrumentation:master May 18, 2021
@aabmass
Copy link
Member

aabmass commented May 18, 2021

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants