-
Notifications
You must be signed in to change notification settings - Fork 314
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
fix(destination): oauth stats prefix #4033
Conversation
Signed-off-by: Sai Sankeerth <sanpj2292@github.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.
GetTokenInfo
is also creating stats like this:
defer func() {
authStats.statName = fmt.Sprintf("%v_total_req_latency", refTokenParams.EventNamePrefix)
// ...
}()
I know that it's called at the moment by FetchToken
and RefreshToken
only but the method is part of the public API. Since it is exported, someone might call this method and pass an empty prefix and break the metrics again.
Also the approach seems a bit fragile in my opinion, why not just do:
defer func() {
authStats.statName = "total_req_latency"
if refTokenParams.EventNamePrefix != "" {
authStats.statName = refTokenParams.EventNamePrefix + "_" + authStats.statName
}
// ...
}()
This way we would also decrease the risk of other people breaking this code.
There is no public interface to call Let me know your thoughts |
If we just want to know who is the caller wouldn't it be better to pass it as a tag rather than being present in stat name? |
What about @cisse21 proposal? |
Yes. It makes sense to send them as tags. Changing this now, doesn't make a lot of sense(I mean more from re-work point-of-view for a small change) but I can work on a fix that would make sure we would not get empty There is an item to re-work on OAuth implementation where in this point will be kept in mind & then re-worked accordingly |
If say there is dis-comfort about having |
@sanpj2292 wdyt of this instead? #4044
It's not about discomfort, it's effectively a private method but it's exported. The package API should be designed by keeping in mind what is accessible from outside and what is not. That method should not be accessible, right? |
- include optional tags - introduce action tag - change stat names for token related ones - change stat name for authStatus related ones - remove eventNamePrefix
@fracasula @cisse21 I have made changes as requested, can you guys please check and let me know if they look good ? |
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 can't find any calls to SendTimerStats
or SendCountStats
with tags.
Wouldn't it make sense to just leave the metric names simpler (i.e. instead of token_request_latency
=> request_latency
) and then pass a tag like action=fetch_token
or refresh_token
, etc...?
This is done to add more flexibility. Can remove it as it is not being used
It feels incomplete for some of the stats. Instead i've added |
Removed it |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4033 +/- ##
==========================================
+ Coverage 71.75% 71.84% +0.08%
==========================================
Files 374 374
Lines 54919 54916 -3
==========================================
+ Hits 39407 39452 +45
+ Misses 13188 13141 -47
+ Partials 2324 2323 -1
☔ View full report in Codecov by Sentry. |
Description
Prefix for some of the stats in OAuth module was missing. We have made some defaults instead of relying on the ones that are sent as arguments.
Linear Ticket
Resolves INT-939
Security