-
Notifications
You must be signed in to change notification settings - Fork 0
fix(tracing): actually use span on permissioning #48
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
Conversation
We were not actually entering the span, so the logs show up pretty empty.
#[error( | ||
"builder not permissioned for this slot: requesting builder {0}, permissioned builder {1}" | ||
)] | ||
NotPermissioned(String, String), |
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.
@prestwich you ended up being right here that the messages on this error should've been made more useful. This was merged but we can keep working on this on a new PR.
return Err(BuilderPermissionError::NotPermissioned); | ||
return Err(BuilderPermissionError::NotPermissioned( | ||
sub.to_owned(), | ||
self.current_builder().sub.to_owned(), |
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.
subs are not secrets right @rswanson
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.
correct
@@ -187,6 +194,8 @@ where | |||
|
|||
info!("builder permissioned successfully"); | |||
|
|||
drop(guard); | |||
|
|||
this.inner.call(req).await |
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.
should we instrument this future with the perms span?
ugh didn't see this was merged before commenting |
We were not actually entering the span, so the logs show up pretty empty.