-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add tracing spans for rules API #4178
Add tracing spans for rules API #4178
Conversation
Signed-off-by: Matsumoto <toshi.matsumoto.3n@gmail.com>
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.
LGTM 🥇 Thanks for your contribution.
Great, clean PR.
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.
Overall it looks good. But one thing I am not sure about is that it is too fine-grained, which might create a lot of spans for a single Rules request.
I am not very familiar with trace instrumentation, is there a best practice about it?
Signed-off-by: Matsumoto <toshi.matsumoto.3n@gmail.com>
I don't know the best practices either, but... |
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.
Thanks a lot! Just one nit (:
I think this amount is fine.
pkg/rules/manager.go
Outdated
func (m *Manager) Rules(r *rulespb.RulesRequest, s rulespb.Rules_RulesServer) error { | ||
var err error |
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.
func (m *Manager) Rules(r *rulespb.RulesRequest, s rulespb.Rules_RulesServer) error { | |
var err error | |
func (m *Manager) Rules(r *rulespb.RulesRequest, s rulespb.Rules_RulesServer) (err error) { |
We can simplify (:
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.
I've finished fixing it.
Hi @toshi1127, please rebase on main then we can get CI fixed. |
Signed-off-by: Matsumoto <toshi.matsumoto.3n@gmail.com>
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.
LGTM!
Signed-off-by: Matsumoto toshi.matsumoto.3n@gmail.com
Changes
Add tracing for federated rules API.
Closes #4131
Verification
make build
make test-local