Skip to content
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.

Sign up for GitHub

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

Improve gorouter Route Registry Message Metrics #456

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+311 −178
Open

Improve gorouter Route Registry Message Metrics #456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
90b997f
Refactor code to unregister endpoints and routes
hoffmaen Dec 11, 2024
2b89f38
Introduce disctinct metrics for each route (un)registration case
hoffmaen Dec 12, 2024
8c017fb
Fix tests
hoffmaen Dec 12, 2024
7271255
Use Metric Batcher for Unregisty Message
hoffmaen Dec 13, 2024
7e52d8d
Rename constants for pool update events
hoffmaen Dec 13, 2024
8ec4a99
Fix tests with Metrics Batcher
hoffmaen Dec 13, 2024
e14d71e
Use constants for log and metrics messages
hoffmaen Dec 13, 2024
ef4bebb
fix: Refactor Register, fix log levels
b1tamara Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions handlers/routeservice_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ var _ = Describe("Route Service Handler", func() {
endpoint := route.NewEndpoint(&route.EndpointOpts{})

added := routePool.Put(endpoint)
Expect(added).To(Equal(route.ADDED))
Expect(added).To(Equal(route.EndpointAdded))
})
It("should not add route service metadata to the request for normal routes", func() {
handler.ServeHTTP(resp, req)
Expand All @@ -152,7 +152,7 @@ var _ = Describe("Route Service Handler", func() {
BeforeEach(func() {
endpoint := route.NewEndpoint(&route.EndpointOpts{RouteServiceUrl: "route-service.com"})
added := routePool.Put(endpoint)
Expect(added).To(Equal(route.ADDED))
Expect(added).To(Equal(route.EndpointAdded))
})

It("returns 502 Bad Gateway", func() {
Expand All @@ -172,7 +172,7 @@ var _ = Describe("Route Service Handler", func() {
BeforeEach(func() {
endpoint := route.NewEndpoint(&route.EndpointOpts{})
added := routePool.Put(endpoint)
Expect(added).To(Equal(route.ADDED))
Expect(added).To(Equal(route.EndpointAdded))
})
It("should not add route service metadata to the request for normal routes", func() {
handler.ServeHTTP(resp, req)
Expand Down Expand Up @@ -211,7 +211,7 @@ var _ = Describe("Route Service Handler", func() {
BeforeEach(func() {
endpoint := route.NewEndpoint(&route.EndpointOpts{RouteServiceUrl: "https://route-service.com"})
added := routePool.Put(endpoint)
Expect(added).To(Equal(route.ADDED))
Expect(added).To(Equal(route.EndpointAdded))
})

It("sends the request to the route service with X-CF-Forwarded-Url using https scheme", func() {
Expand Down Expand Up @@ -699,7 +699,7 @@ var _ = Describe("Route Service Handler", func() {
endpoint := route.NewEndpoint(&route.EndpointOpts{RouteServiceUrl: "https://goodrouteservice.com"})

added := routePool.Put(endpoint)
Expect(added).To(Equal(route.ADDED))
Expect(added).To(Equal(route.EndpointAdded))
req.Header.Set("connection", "upgrade")
req.Header.Set("upgrade", "websocket")

Expand All @@ -718,7 +718,7 @@ var _ = Describe("Route Service Handler", func() {
BeforeEach(func() {
endpoint := route.NewEndpoint(&route.EndpointOpts{RouteServiceUrl: "https://bad%20service.com"})
added := routePool.Put(endpoint)
Expect(added).To(Equal(route.ADDED))
Expect(added).To(Equal(route.EndpointAdded))

})
It("returns a 500 internal server error response", func() {
Expand Down
4 changes: 2 additions & 2 deletions metrics/compositereporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ type RouteRegistryReporter interface {
CaptureRouteStats(totalRoutes int, msSinceLastUpdate int64)
CaptureRoutesPruned(prunedRoutes uint64)
CaptureLookupTime(t time.Duration)
CaptureRegistryMessage(msg ComponentTagged)
CaptureRegistryMessage(msg ComponentTagged, action string)
CaptureRouteRegistrationLatency(t time.Duration)
UnmuzzleRouteRegistrationLatency()
CaptureUnregistryMessage(msg ComponentTagged)
CaptureUnregistryMessage(msg ComponentTagged, action string)
}

type CompositeReporter struct {
Expand Down
36 changes: 20 additions & 16 deletions metrics/fakes/fake_registry_reporter.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 8 additions & 13 deletions metrics/metricsreporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,27 +135,22 @@ func (m *MetricsReporter) CaptureRoutesPruned(routesPruned uint64) {
m.Batcher.BatchAddCounter("routes_pruned", routesPruned)
}

func (m *MetricsReporter) CaptureRegistryMessage(msg ComponentTagged) {
func (m *MetricsReporter) CaptureRegistryMessage(msg ComponentTagged, action string) {
var componentName string
if msg.Component() == "" {
componentName = "registry_message"
componentName = "registry_message." + action
} else {
componentName = "registry_message." + msg.Component()
componentName = "registry_message." + action + "." + msg.Component()
Copy link
Member

@maxmoehl maxmoehl Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably also change the metric name right? I was hoping that we could preserve the metric name but only add an additional tag to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I had in mind. But it seems like the API is a bit cumbersome to use...

}
m.Batcher.BatchIncrementCounter(componentName)
}

func (m *MetricsReporter) CaptureUnregistryMessage(msg ComponentTagged) {
var componentName string
if msg.Component() == "" {
componentName = "unregistry_message"
} else {
componentName = "unregistry_message." + msg.Component()
}
err := m.Sender.IncrementCounter(componentName)
if err != nil {
m.Logger.Debug("failed-sending-metric", log.ErrAttr(err), slog.String("metric", componentName))
func (m *MetricsReporter) CaptureUnregistryMessage(msg ComponentTagged, action string) {
unregisterMsg := "unregistry_message." + action
if msg.Component() != "" {
unregisterMsg = unregisterMsg + "." + msg.Component()
}
m.Batcher.BatchIncrementCounter(unregisterMsg)
}

func (m *MetricsReporter) CaptureWebSocketUpdate() {
Expand Down
32 changes: 16 additions & 16 deletions metrics/metricsreporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,22 +448,22 @@ var _ = Describe("MetricsReporter", func() {

It("sends number of nats messages received from each component", func() {
endpoint.Tags = map[string]string{}
metricReporter.CaptureRegistryMessage(endpoint)
metricReporter.CaptureRegistryMessage(endpoint, "some-action")

Expect(batcher.BatchIncrementCounterCallCount()).To(Equal(1))
Expect(batcher.BatchIncrementCounterArgsForCall(0)).To(Equal("registry_message"))
Expect(batcher.BatchIncrementCounterArgsForCall(0)).To(Equal("registry_message.some-action"))
})

It("sends number of nats messages received from each component", func() {
endpoint.Tags = map[string]string{"component": "uaa"}
metricReporter.CaptureRegistryMessage(endpoint)
metricReporter.CaptureRegistryMessage(endpoint, "some-action")

endpoint.Tags = map[string]string{"component": "route-emitter"}
metricReporter.CaptureRegistryMessage(endpoint)
metricReporter.CaptureRegistryMessage(endpoint, "some-action")

Expect(batcher.BatchIncrementCounterCallCount()).To(Equal(2))
Expect(batcher.BatchIncrementCounterArgsForCall(0)).To(Equal("registry_message.uaa"))
Expect(batcher.BatchIncrementCounterArgsForCall(1)).To(Equal("registry_message.route-emitter"))
Expect(batcher.BatchIncrementCounterArgsForCall(0)).To(Equal("registry_message.some-action.uaa"))
Expect(batcher.BatchIncrementCounterArgsForCall(1)).To(Equal("registry_message.some-action.route-emitter"))
})

It("sends the total routes", func() {
Expand Down Expand Up @@ -517,33 +517,33 @@ var _ = Describe("MetricsReporter", func() {
BeforeEach(func() {
endpoint = new(route.Endpoint)
endpoint.Tags = map[string]string{"component": "oauth-server"}
metricReporter.CaptureUnregistryMessage(endpoint)
metricReporter.CaptureUnregistryMessage(endpoint, "some-action")
})

It("increments the counter metric", func() {
Expect(sender.IncrementCounterCallCount()).To(Equal(1))
Expect(sender.IncrementCounterArgsForCall(0)).To(Equal("unregistry_message.oauth-server"))
Expect(batcher.BatchIncrementCounterCallCount()).To(Equal(1))
Expect(batcher.BatchIncrementCounterArgsForCall(0)).To(Equal("unregistry_message.some-action.oauth-server"))
})

It("increments the counter metric for each component unregistered", func() {
endpointTwo := new(route.Endpoint)
endpointTwo.Tags = map[string]string{"component": "api-server"}
metricReporter.CaptureUnregistryMessage(endpointTwo)
metricReporter.CaptureUnregistryMessage(endpointTwo, "some-action")

Expect(sender.IncrementCounterCallCount()).To(Equal(2))
Expect(sender.IncrementCounterArgsForCall(0)).To(Equal("unregistry_message.oauth-server"))
Expect(sender.IncrementCounterArgsForCall(1)).To(Equal("unregistry_message.api-server"))
Expect(batcher.BatchIncrementCounterCallCount()).To(Equal(2))
Expect(batcher.BatchIncrementCounterArgsForCall(0)).To(Equal("unregistry_message.some-action.oauth-server"))
Expect(batcher.BatchIncrementCounterArgsForCall(1)).To(Equal("unregistry_message.some-action.api-server"))
})
})
Context("when unregister msg with empty component name is incremented", func() {
BeforeEach(func() {
endpoint = new(route.Endpoint)
endpoint.Tags = map[string]string{}
metricReporter.CaptureUnregistryMessage(endpoint)
metricReporter.CaptureUnregistryMessage(endpoint, "some-action")
})
It("increments the counter metric", func() {
Expect(sender.IncrementCounterCallCount()).To(Equal(1))
Expect(sender.IncrementCounterArgsForCall(0)).To(Equal("unregistry_message"))
Expect(batcher.BatchIncrementCounterCallCount()).To(Equal(1))
Expect(batcher.BatchIncrementCounterArgsForCall(0)).To(Equal("unregistry_message.some-action"))
})
})
})
Expand Down
Loading