Fix most of Peter's comments

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Author: Ganesh Vernekar

development/tsdb-blocks-storage-s3/config/cortex.yaml

@@ -88,7 +88,7 @@ alertmanager:
   enable_api: true
   sharding_enabled: true
   sharding_ring:
-    replication_factor: 1
+    replication_factor: 3
     heartbeat_period: 5s
     heartbeat_timeout: 15s
     kvstore:

development/tsdb-blocks-storage-s3/docker-compose.yml

@@ -215,18 +215,48 @@ services:
     volumes:
       - ./config:/cortex/config
 
-  alertmanager:
+  alertmanager-1:
     build:
       context:    .
       dockerfile: dev.dockerfile
     image: cortex
-    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18010 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8010 -server.grpc-listen-port=9010 -alertmanager.web.external-url=localhost:8010"]
+    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18031 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8031 -server.grpc-listen-port=9031 -alertmanager.web.external-url=localhost:8031"]
     depends_on:
       - consul
       - minio
     ports:
-      - 8010:8010
-      - 18010:18010
+      - 8031:8031
+      - 18031:18031
+    volumes:
+      - ./config:/cortex/config
+
+  alertmanager-2:
+    build:
+      context:    .
+      dockerfile: dev.dockerfile
+    image: cortex
+    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18032 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8032 -server.grpc-listen-port=9032 -alertmanager.web.external-url=localhost:8032"]
+    depends_on:
+      - consul
+      - minio
+    ports:
+      - 8032:8032
+      - 18032:18032
+    volumes:
+      - ./config:/cortex/config
+
+  alertmanager-3:
+    build:
+      context:    .
+      dockerfile: dev.dockerfile
+    image: cortex
+    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18033 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8033 -server.grpc-listen-port=9033 -alertmanager.web.external-url=localhost:8033"]
+    depends_on:
+      - consul
+      - minio
+    ports:
+      - 8033:8033
+      - 18033:18033
     volumes:
       - ./config:/cortex/config
 

pkg/alertmanager/alertmanager_client.go

@@ -64,11 +64,9 @@ func newAlertmanagerClientsPool(discovery client.PoolServiceDiscovery, amClientC
 	}
 
 	requestDuration := promauto.With(reg).NewHistogramVec(prometheus.HistogramOpts{
-		Namespace:   "cortex",
-		Name:        "alertmanager_distributor_client_request_duration_seconds",
-		Help:        "Time spent executing requests from an alertmanager to another alertmanager.",
-		Buckets:     prometheus.ExponentialBuckets(0.008, 4, 7),
-		ConstLabels: prometheus.Labels{"client": "alertmanager"},
+		Name:    "cortex_alertmanager_distributor_client_request_duration_seconds",
+		Help:    "Time spent executing requests from an alertmanager to another alertmanager.",
+		Buckets: prometheus.ExponentialBuckets(0.008, 4, 7),
 	}, []string{"operation", "status_code"})
 
 	factory := func(addr string) (client.PoolClient, error) {

pkg/alertmanager/distributor.go

@@ -145,8 +145,8 @@ func (d *Distributor) doWrite(userID string, w http.ResponseWriter, r *http.Requ
 		respondFromHTTPGRPCResponse(w, firstSuccessfulResponse)
 	} else {
 		// This should not happen.
-		level.Warn(logger).Log("msg", "distributor did not receive response from alertmanager though no errors")
-		w.WriteHeader(http.StatusOK)
+		level.Error(logger).Log("msg", "distributor did not receive response from alertmanager though no errors")
+		w.WriteHeader(http.StatusInternalServerError)
 	}
 }
 

pkg/alertmanager/multitenant.go

@@ -237,9 +237,15 @@ type MultitenantAlertmanager struct {
 	cfg *MultitenantAlertmanagerConfig
 
 	// Ring used for sharding alertmanager instances.
+	// When sharding is disabled, the flow is:
+	//   ServeHTTP() -> serveRequest()
+	// When sharding is enabled:
+	//   ServeHTTP() -> distributor.DistributeRequest() -> (sends to other AM or even the current)
+	//     -> HandleRequest() (gRPC call) -> grpcServer() -> handlerForGRPCServer.ServeHTTP() -> serveRequest().
 	ringLifecycler *ring.BasicLifecycler
 	ring           *ring.Ring
 	distributor    *Distributor
+	grpcServer     *server.Server
 
 	// Subservices manager (ring, lifecycler)
 	subservices        *services.Manager
@@ -265,7 +271,6 @@ type MultitenantAlertmanager struct {
 	peer *cluster.Peer
 
 	// For distributor.
-	grpcServer *server.Server
 
 	registry          prometheus.Registerer
 	ringCheckErrors   prometheus.Counter
@@ -428,12 +433,14 @@ func createMultitenantAlertmanager(cfg *MultitenantAlertmanagerConfig, fallbackC
 	return am, nil
 }
 
+// handlerForGRPCServer acts as a handler for gRPC server to serve
+// the serveRequest() via the standard ServeHTTP.
 type handlerForGRPCServer struct {
 	am *MultitenantAlertmanager
 }
 
 func (h *handlerForGRPCServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
-	h.am.serveHTTPHelper(w, req)
+	h.am.serveRequest(w, req)
 }
 
 func (am *MultitenantAlertmanager) starting(ctx context.Context) (err error) {
@@ -770,16 +777,16 @@ func (am *MultitenantAlertmanager) ServeHTTP(w http.ResponseWriter, req *http.Re
 
 	// If sharding is not enabled or Distributor does not support this path,
 	// it is served by this instance.
-	am.serveHTTPHelper(w, req)
+	am.serveRequest(w, req)
 }
 
-// HandleRequest serves the Alertmanager's web UI and API sent via gRPC.
+// HandleRequest implements gRPC Alertmanager service, which receives request from AlertManager-Distributor.
 func (am *MultitenantAlertmanager) HandleRequest(ctx context.Context, in *httpgrpc.HTTPRequest) (*httpgrpc.HTTPResponse, error) {
 	return am.grpcServer.Handle(ctx, in)
 }
 
-// serveHTTPHelper serves the Alertmanager's web UI and API.
-func (am *MultitenantAlertmanager) serveHTTPHelper(w http.ResponseWriter, req *http.Request) {
+// serveRequest serves the Alertmanager's web UI and API.
+func (am *MultitenantAlertmanager) serveRequest(w http.ResponseWriter, req *http.Request) {
 	userID, err := tenant.TenantID(req.Context())
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusUnauthorized)