Conversation
| client: &Client, | ||
| database: &Database, | ||
| region: &Region, | ||
| telemetry: Option<&crate::telemetry::TelemetryThread>, |
There was a problem hiding this comment.
Does this need to be an Option type? Do we not want to always emit something when token refresh fails?
There was a problem hiding this comment.
Its because of the load function. There are cases when we can pass None to it for telemetry because its not a login failure case. And hence that gets passed down. So we just implemented it to be able to handle a None for telemetry.
There was a problem hiding this comment.
Oh alright. I'll let the second PR chime in here. Not familiar enough with the auth flow to have more insights here.
There was a problem hiding this comment.
This is more like a code arrangement thing now. Either I handle the None in one place vs the other.
There was a problem hiding this comment.
imo, instead of making this an Option, you can just check if there's an error and only then publish the auth-failed?
Also, do you wanna publish an auth-succeeded metric?
There was a problem hiding this comment.
So the issue here is that load is being called in a-lot of places which may not necessarily need telemetry publish. So instead of adding the code everywhere to have telemetry to pass, I am allowing a None. We are already only posting auth_failed only when we have an error.
We already have a login successful metric. So that covers it.
| } | ||
| } | ||
|
|
||
| pub fn token_type_from_start_url(start_url: Option<&str>) -> TokenType { |
There was a problem hiding this comment.
Maybe just implement an From<Option<&str>> for TokenType here. We own the definition of TokenType in this crate so you don't have to worry about orphan rule.
There was a problem hiding this comment.
You are right. Will take care of this.
3 participants
We didnt have good telemetry for auth login failures. Added it here.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.