[server, papi] Track login completed metrics by outcome, type (#18254)

Co-authored-by: geropl <[email protected]>

Author: easyCZ

components/public-api-server/pkg/oidc/metrics.go

@@ -0,0 +1,25 @@
+// Copyright (c) 2023 Gitpod GmbH. All rights reserved.
+// Licensed under the GNU Affero General Public License (AGPL).
+// See License.AGPL.txt in the project root for license information.
+
+package oidc
+
+import (
+	"github.com/prometheus/client_golang/prometheus"
+)
+
+var (
+	loginCompletedTotal = prometheus.NewCounterVec(prometheus.CounterOpts{
+		Namespace: "gitpod",
+		Name:      "login_completed_total",
+		Help:      "Total number of logins completed into gitpod, by status",
+	}, []string{"status", "type"})
+)
+
+func RegisterMetrics(registry *prometheus.Registry) {
+	registry.MustRegister(loginCompletedTotal)
+}
+
+func reportLoginCompleted(status string, typez string) {
+	loginCompletedTotal.WithLabelValues(status, typez).Inc()
+}

components/public-api-server/pkg/oidc/router.go

@@ -111,18 +111,21 @@ func (s *Service) getCallbackHandler() http.HandlerFunc {
 		useHttpErrors := state.UseHttpErrors
 		if err != nil {
 			log.WithError(err).Warn("Client SSO config not found")
+			reportLoginCompleted("failed_client", "sso")
 			respondeWithError(rw, r, "We were unable to find the SSO configuration you've requested. Please verify SSO is configured with your Organization owner.", http.StatusNotFound, useHttpErrors)
 			return
 		}
 		oauth2Result := GetOAuth2ResultFromContext(r.Context())
 		if oauth2Result == nil {
+			reportLoginCompleted("failed_client", "sso")
 			respondeWithError(rw, r, "OIDC precondition failure", http.StatusInternalServerError, useHttpErrors)
 			return
 		}
 
 		// nonce = number used once
 		nonceCookie, err := r.Cookie(nonceCookieName)
 		if err != nil {
+			reportLoginCompleted("failed_client", "sso")
 			respondeWithError(rw, r, "There was no nonce present on the request. Please try to sign-in in again.", http.StatusBadRequest, useHttpErrors)
 			return
 		}
@@ -133,6 +136,7 @@ func (s *Service) getCallbackHandler() http.HandlerFunc {
 		})
 		if err != nil {
 			log.WithError(err).Warn("OIDC authentication failed")
+			reportLoginCompleted("failed_client", "sso")
 			respondeWithError(rw, r, "We've not been able to authenticate you with the OIDC Provider.", http.StatusInternalServerError, useHttpErrors)
 			return
 		}
@@ -143,34 +147,35 @@ func (s *Service) getCallbackHandler() http.HandlerFunc {
 			err = s.activateAndVerifyClientConfig(r.Context(), config)
 			if err != nil {
 				log.WithError(err).Warn("Failed to mark OIDC Client Config as active")
+				reportLoginCompleted("failed", "sso")
 				respondeWithError(rw, r, "We've been unable to mark the selected OIDC config as active. Please try again.", http.StatusInternalServerError, useHttpErrors)
 				return
 			}
 		}
 
 		if state.Verify {
+			// Skip the sign-in on verify-only requests.
 			err = s.markClientConfigAsVerified(r.Context(), config)
 			if err != nil {
 				log.Warn("Failed to mark config as verified: " + err.Error())
+				reportLoginCompleted("failed", "sso")
 				respondeWithError(rw, r, "Failed to mark config as verified", http.StatusInternalServerError, useHttpErrors)
 				return
 			}
-		}
-
-		// Skip the sign-in on verify-only requests.
-		if !state.Verify {
+		} else {
 			cookies, _, err := s.createSession(r.Context(), result, config)
 			if err != nil {
 				log.WithError(err).Warn("Failed to create session from downstream session provider.")
+				reportLoginCompleted("failed", "sso")
 				respondeWithError(rw, r, "We were unable to create a user session.", http.StatusInternalServerError, useHttpErrors)
 				return
 			}
 			for _, cookie := range cookies {
 				http.SetCookie(rw, cookie)
 			}
-
 		}
 
+		reportLoginCompleted("succeeded", "sso")
 		http.Redirect(rw, r, oauth2Result.ReturnToURL, http.StatusTemporaryRedirect)
 	}
 }

components/public-api-server/pkg/server/server.go

@@ -174,6 +174,7 @@ type registerDependencies struct {
 
 func register(srv *baseserver.Server, deps *registerDependencies) error {
 	proxy.RegisterMetrics(srv.MetricsRegistry())
+	oidc.RegisterMetrics(srv.MetricsRegistry())
 
 	connectMetrics := NewConnectMetrics()
 	err := connectMetrics.Register(srv.MetricsRegistry())

components/server/src/auth/authenticator.ts

@@ -16,8 +16,8 @@ import { AuthFlow, AuthProvider } from "./auth-provider";
 import { TokenProvider } from "../user/token-provider";
 import { AuthProviderService } from "./auth-provider-service";
 import { UserAuthentication } from "../user/user-authentication";
-import { increaseLoginCounter } from "../prometheus-metrics";
 import { SignInJWT } from "./jwt";
+import { reportLoginCompleted } from "../prometheus-metrics";
 
 @injectable()
 export class Authenticator {
@@ -165,8 +165,8 @@ export class Authenticator {
         }
 
         if (!authProvider.info.verified) {
-            increaseLoginCounter("failed", authProvider.info.host);
-            log.info(`Login with "${host}" is not permitted.`, {
+            reportLoginCompleted("failed_client", "git");
+            log.warn(`Login with "${host}" is not permitted as the provider has not been verified.`, {
                 "login-flow": true,
                 ap: authProvider.info,
             });

components/server/src/auth/generic-auth-provider.ts

@@ -27,14 +27,14 @@ import { TokenProvider } from "../user/token-provider";
 import { UserAuthentication } from "../user/user-authentication";
 import { AuthProviderService } from "./auth-provider-service";
 import { LoginCompletionHandler } from "./login-completion-handler";
-import { increaseLoginCounter } from "../prometheus-metrics";
 import { OutgoingHttpHeaders } from "http2";
 import { trackSignup } from "../analytics";
 import { daysBefore, isDateSmaller } from "@gitpod/gitpod-protocol/lib/util/timeutil";
 import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics";
 import { VerificationService } from "../auth/verification-service";
 import { SignInJWT } from "./jwt";
 import { UserService } from "../user/user-service";
+import { reportLoginCompleted } from "../prometheus-metrics";
 
 /**
  * This is a generic implementation of OAuth2-based AuthProvider.
@@ -284,7 +284,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
         if (!authFlow) {
             log.error(`(${strategyName}) Auth flow state is missing.`);
 
-            increaseLoginCounter("failed", this.host);
+            reportLoginCompleted("failed", "git");
             response.redirect(this.getSorryUrl(`Auth flow state is missing.`));
             return;
         }
@@ -304,15 +304,15 @@ export abstract class GenericAuthProvider implements AuthProvider {
         // assert additional infomation is attached to current session
         if (!authFlow) {
             // The auth flow state info is missing in the session: count as client error
-            increaseLoginCounter("failed_client", this.host);
+            reportLoginCompleted("failed_client", "git");
 
             log.error(cxt, `(${strategyName}) No session found during auth callback.`, { clientInfo });
             response.redirect(this.getSorryUrl(`Please allow Cookies in your browser and try to log in again.`));
             return;
         }
 
         if (authFlow.host !== this.host) {
-            increaseLoginCounter("failed", this.host);
+            reportLoginCompleted("failed", "git");
 
             log.error(cxt, `(${strategyName}) Host does not match.`, { clientInfo });
             response.redirect(this.getSorryUrl(`Host does not match.`));
@@ -328,8 +328,8 @@ export abstract class GenericAuthProvider implements AuthProvider {
 
         if (callbackError) {
             // e.g. "access_denied"
+            reportLoginCompleted("failed", "git");
 
-            increaseLoginCounter("failed", this.host);
             return this.sendCompletionRedirectWithError(response, {
                 error: callbackError,
                 description: callbackErrorDescription,
@@ -398,7 +398,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
                 return this.sendCompletionRedirectWithError(response, { error: err.message });
             }
 
-            increaseLoginCounter("failed", this.host);
+            reportLoginCompleted("failed", "git");
             log.error(context, `(${strategyName}) Redirect to /sorry from verify callback`, err, {
                 ...defaultLogPayload,
                 err,

components/server/src/auth/login-completion-handler.ts

@@ -11,7 +11,7 @@ import { log, LogContext } from "@gitpod/gitpod-protocol/lib/util/logging";
 import { Config } from "../config";
 import { HostContextProvider } from "./host-context-provider";
 import { AuthProviderService } from "./auth-provider-service";
-import { increaseLoginCounter, reportJWTCookieIssued } from "../prometheus-metrics";
+import { reportJWTCookieIssued, reportLoginCompleted } from "../prometheus-metrics";
 import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics";
 import { trackLogin } from "../analytics";
 import { SessionHandler } from "../session-handler";
@@ -47,10 +47,8 @@ export class LoginCompletionHandler {
                 });
             });
         } catch (err) {
-            if (authHost) {
-                increaseLoginCounter("failed", authHost);
-            }
-            log.error(logContext, `Redirect to /sorry on login`, err, { err });
+            reportLoginCompleted("failed", "git");
+            log.error(logContext, `Failed to login user. Redirecting to /sorry on login.`, err);
             response.redirect(this.config.hostUrl.asSorry("Oops! Something went wrong during login.").toString());
             return;
         }
@@ -75,8 +73,6 @@ export class LoginCompletionHandler {
         }
 
         if (authHost) {
-            increaseLoginCounter("succeeded", authHost);
-
             /** no await */ trackLogin(user, request, authHost, this.analytics).catch((err) =>
                 log.error({ userId: user.id }, "Failed to track Login.", err),
             );
@@ -87,6 +83,7 @@ export class LoginCompletionHandler {
         reportJWTCookieIssued();
 
         log.info(logContext, `User is logged in successfully. Redirect to: ${returnTo}`);
+        reportLoginCompleted("succeeded", "git");
         response.redirect(returnTo);
     }
 

components/server/src/prometheus-metrics.ts

@@ -7,7 +7,7 @@
 import * as prometheusClient from "prom-client";
 
 export function registerServerMetrics(registry: prometheusClient.Registry) {
-    registry.registerMetric(loginCounter);
+    registry.registerMetric(loginCompletedTotal);
     registry.registerMetric(apiConnectionCounter);
     registry.registerMetric(apiConnectionClosedCounter);
     registry.registerMetric(apiCallCounter);
@@ -35,12 +35,16 @@ export function registerServerMetrics(registry: prometheusClient.Registry) {
     registry.registerMetric(updateSubscribersRegistered);
 }
 
-const loginCounter = new prometheusClient.Counter({
-    name: "gitpod_server_login_requests_total",
-    help: "Total amount of login requests",
-    labelNames: ["status", "auth_host"],
+const loginCompletedTotal = new prometheusClient.Counter({
+    name: "gitpod_login_completed_total",
+    help: "Total number of logins completed into gitpod, by status",
+    labelNames: ["status", "type"],
 });
 
+export function reportLoginCompleted(status: LoginCounterStatus, type: "git" | "sso") {
+    loginCompletedTotal.labels(status, type).inc();
+}
+
 type LoginCounterStatus =
     // The login attempt failed due to a system error (picked up by alerts)
     | "failed"
@@ -49,13 +53,6 @@ type LoginCounterStatus =
     // The login attempt failed, because the client failed to provide complete session information, for instance.
     | "failed_client";
 
-export function increaseLoginCounter(status: LoginCounterStatus, auth_host: string) {
-    loginCounter.inc({
-        status,
-        auth_host,
-    });
-}
-
 const apiConnectionCounter = new prometheusClient.Counter({
     name: "gitpod_server_api_connections_total",
     help: "Total amount of established API connections",