-
Notifications
You must be signed in to change notification settings - Fork 280
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
Early push to bring tracing into tower #298
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start, I left some notes that you may find valuable.
I don't think there's a strong reason not to land something like this soon, and then we can iterate on it going forward. |
I would love to be using this in linkerd, fwiw. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM and I'd be happy to see it merge as-is. I left some comments on how we could potentially make more idiomatic use of tracing
by including more structured data in the events we record, feel free to take it or leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM beyond that I think the service count is being reported incorrectly in tower_balance::pool
. Fix that & this is good to merge!
No description provided.