Add instrumentation code for SSO #1003
Conversation
|
New Gopher here so feel free to roast me on styling. It would be greatly appreciated to describe why we follow specific guidelines |
| singleSignOn sso.SingleSignOn, | ||
| webFrontendURL string, | ||
| ) router.Handle { | ||
| return func(w http.ResponseWriter, r *http.Request, params router.Params) { | ||
| i := instrumentationFactory.NewRequest() |
There was a problem hiding this comment.
| i := instrumentationFactory.NewRequest() | |
| i := instrumentationFactory.NewHTTP(r) |
There was a problem hiding this comment.
We are tracking HTTP traffic here.
There was a problem hiding this comment.
This is to prepare for distributed tracing in the future
There was a problem hiding this comment.
The main difference that I see is that its extracting the location from the HTTP call, is this true?
| @@ -10,10 +11,14 @@ import ( | |||
|
|
|||
| // SSOSignIn redirects user to the sign in page. | |||
| func SSOSignIn( | |||
| instrumentationFactory request.InstrumentationFactory, | |||
There was a problem hiding this comment.
| instrumentationFactory request.InstrumentationFactory, | |
| instrumentationFactory request.SSOInstrumentationFactory, |
There was a problem hiding this comment.
Can you explain this a bit further, are we trying to compose an instrumentationFactory within an SSOInstrumentationFactory?
There was a problem hiding this comment.
@Ashakibp Nope. SSO sign in only depends on SSOInstrumentationFactory, which constructs instrumentation.SSO
| singleSignOn sso.SingleSignOn, | ||
| webFrontendURL string, | ||
| ) router.Handle { | ||
| return func(w http.ResponseWriter, r *http.Request, params router.Params) { | ||
| i := instrumentationFactory.NewRequest() | ||
| i.SSOSignIn(singleSignOn.GetMetricName()) | ||
|
|
||
| token := getToken(params) | ||
| if singleSignOn.IsSignedIn(token) { |
There was a problem hiding this comment.
We need to invoke the instructation method here to actually track data.
There was a problem hiding this comment.
Also after this code path, we track the user is redirected to the generated sign in link at line 27.
| singleSignOn sso.SingleSignOn, | ||
| webFrontendURL string, | ||
| ) router.Handle { | ||
| return func(w http.ResponseWriter, r *http.Request, params router.Params) { | ||
| i := instrumentationFactory.NewRequest() | ||
| i.SSOSignIn(singleSignOn.GetMetricName()) |
There was a problem hiding this comment.
The correct InstrumentationFactory can be injected inside the caller of this function.
There was a problem hiding this comment.
Instrumentation code should be decoupled from SSO so that we can change them independently
…e-to-all-routes' into ashakibp-Add-instrumentation-code-to-all-routes
| @@ -10,10 +11,14 @@ import ( | |||
|
|
|||
| // SSOSignIn redirects user to the sign in page. | |||
| func SSOSignIn( | |||
| instrumentationFactory request.InstrumentationFactory, | |||
There was a problem hiding this comment.
@Ashakibp Nope. SSO sign in only depends on SSOInstrumentationFactory, which constructs instrumentation.SSO
| singleSignOn sso.SingleSignOn, | ||
| webFrontendURL string, | ||
| ) router.Handle { | ||
| return func(w http.ResponseWriter, r *http.Request, params router.Params) { | ||
| i := instrumentationFactory.NewRequest() |
…e-to-all-routes' into ashakibp-Add-instrumentation-code-to-all-routes
…e-to-all-routes' into ashakibp-Add-instrumentation-code-to-all-routes
Current Behavior
Currently we do not fire off metrics for SSOs, the objective of this PR is to fire off events whenever someone uses SSO and partition it by provider.
For #695
Description
We used a base instrumentation struct and then an implementation for every provider to make it easier for metric names to be traced down. The factory method will automatically pull the correct provider.