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
pkg/oauth2: consider session cancellation #182
Changes from 1 commit
7c219eb
b4f1fa8
36c72d7
b10ffcf
71867d0
fe20139
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,23 @@ import ( | |
"sync" | ||
"time" | ||
|
||
"github.com/prometheus/client_golang/prometheus" | ||
"golang.org/x/oauth2" | ||
) | ||
|
||
var ( | ||
grantsTotal = prometheus.NewCounter( | ||
prometheus.CounterOpts{ | ||
Name: "password_credentials_grants_total", | ||
Help: "Tracks the number of resource owner password credential grants.", | ||
}, | ||
) | ||
) | ||
|
||
func init() { | ||
prometheus.MustRegister(grantsTotal) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we not use the global registry? I'd like us to generally move away from this pattern, and we should be a role model for the rest of the teams. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea. we can do a head-start here 👍 I suggest to dependency-inject the registry here, then gradually factor all the other places, wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that works for me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed, ptal |
||
} | ||
|
||
type passwordCredentialsTokenSource struct { | ||
ctx context.Context | ||
cfg *oauth2.Config | ||
|
@@ -81,6 +95,8 @@ func (c *passwordCredentialsTokenSource) Token() (*oauth2.Token, error) { | |
} | ||
|
||
func (c *passwordCredentialsTokenSource) passwordCredentialsToken() (*oauth2.Token, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn’t happen too often, but it would be good to know when it does. Let’s add a counter metric to understand how often this happens. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed 👍 added a metric, PTAL |
||
grantsTotal.Inc() | ||
|
||
tok, err := c.cfg.PasswordCredentialsToken(c.ctx, c.username, c.password) | ||
if err != nil { | ||
return nil, fmt.Errorf("password credentials token source failed: %v", err) | ||
|
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 should have a
telemeter_
prefixThere 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.
fixed, ptal