Skip to content

feat!: group 404s without a matching route under one url label by default #5

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

Open
brommell wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat!: group 404s without a matching route under one url label by default #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 9 additions & 4 deletions prometheus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,13 @@ type MiddlewareConfig struct {

timeNow func() time.Time

// If DoNotUseRequestPathFor404 is true, all 404 responses (due to non-matching route) will have the same `url` label and
// thus won't generate new metrics.
// UseRequestPathFor404 sets the `url` label of 404 responses due to non-matching route to the request path.
// By default, all such responses share the same empty `url` label. Enabling this creates a new set of metrics for
// every unknown path requested (scanners and bots), and request paths can contain IDs or personal data.
UseRequestPathFor404 bool

// Deprecated: has no effect, 404 responses due to non-matching route share the same `url` label by default.
// See UseRequestPathFor404.
DoNotUseRequestPathFor404 bool
}

Expand Down Expand Up @@ -253,8 +258,8 @@ func (conf MiddlewareConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
resp, status := echo.ResolveResponseStatus(c.Response(), err)

url := c.Path() // contains route path ala `/users/:id`
if url == "" && !conf.DoNotUseRequestPathFor404 {
// as of Echo v4.10.1 path is empty for 404 cases (when router did not find any matching routes)
if url == "" && conf.UseRequestPathFor404 {
// path is empty for 404 cases (when router did not find any matching routes)
// in this case we use actual path from request to have some distinction in Prometheus
url = c.Request().URL.Path
}
Expand Down
44 changes: 31 additions & 13 deletions prometheus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestCustomRegistryMetrics(t *testing.T) {

s, code := requestBody(e, "/metrics")
assert.Equal(t, http.StatusOK, code)
assert.Contains(t, s, `echo_request_duration_seconds_count{code="404",host="example.com",method="GET",url="/ping"} 1`)
assert.Contains(t, s, `echo_request_duration_seconds_count{code="404",host="example.com",method="GET",url=""} 1`)
}

func TestDefaultRegistryMetrics(t *testing.T) {
Expand All @@ -43,7 +43,7 @@ func TestDefaultRegistryMetrics(t *testing.T) {

s, code := requestBody(e, "/metrics")
assert.Equal(t, http.StatusOK, code)
assert.Contains(t, s, `myapp_request_duration_seconds_count{code="404",host="example.com",method="GET",url="/ping"} 1`)
assert.Contains(t, s, `myapp_request_duration_seconds_count{code="404",host="example.com",method="GET",url=""} 1`)

unregisterDefaults("myapp")
}
Expand All @@ -59,12 +59,12 @@ func TestPrometheus_Buckets(t *testing.T) {

body, code := requestBody(e, "/metrics")
assert.Equal(t, http.StatusOK, code)
assert.Contains(t, body, `echo_request_duration_seconds_bucket{code="404",host="example.com",method="GET",url="/ping",le="0.005"}`, "duration should have time bucket (like, 0.005s)")
assert.NotContains(t, body, `echo_request_duration_seconds_bucket{code="404",host="example.com",method="GET",url="/ping",le="512000"}`, "duration should NOT have a size bucket (like, 512K)")
assert.Contains(t, body, `echo_request_size_bytes_bucket{code="404",host="example.com",method="GET",url="/ping",le="1024"}`, "request size should have a 1024k (size) bucket")
assert.NotContains(t, body, `echo_request_size_bytes_bucket{code="404",host="example.com",method="GET",url="/ping",le="0.005"}`, "request size should NOT have time bucket (like, 0.005s)")
assert.Contains(t, body, `echo_response_size_bytes_bucket{code="404",host="example.com",method="GET",url="/ping",le="1024"}`, "response size should have a 1024k (size) bucket")
assert.NotContains(t, body, `echo_response_size_bytes_bucket{code="404",host="example.com",method="GET",url="/ping",le="0.005"}`, "response size should NOT have time bucket (like, 0.005s)")
assert.Contains(t, body, `echo_request_duration_seconds_bucket{code="404",host="example.com",method="GET",url="",le="0.005"}`, "duration should have time bucket (like, 0.005s)")
assert.NotContains(t, body, `echo_request_duration_seconds_bucket{code="404",host="example.com",method="GET",url="",le="512000"}`, "duration should NOT have a size bucket (like, 512K)")
assert.Contains(t, body, `echo_request_size_bytes_bucket{code="404",host="example.com",method="GET",url="",le="1024"}`, "request size should have a 1024k (size) bucket")
assert.NotContains(t, body, `echo_request_size_bytes_bucket{code="404",host="example.com",method="GET",url="",le="0.005"}`, "request size should NOT have time bucket (like, 0.005s)")
assert.Contains(t, body, `echo_response_size_bytes_bucket{code="404",host="example.com",method="GET",url="",le="1024"}`, "response size should have a 1024k (size) bucket")
assert.NotContains(t, body, `echo_response_size_bytes_bucket{code="404",host="example.com",method="GET",url="",le="0.005"}`, "response size should NOT have time bucket (like, 0.005s)")
}

func TestMiddlewareConfig_Skipper(t *testing.T) {
Expand Down Expand Up @@ -95,8 +95,8 @@ func TestMiddlewareConfig_Skipper(t *testing.T) {

body := out.String()
assert.Contains(t, body, `echo_request_duration_seconds_count{code="200",host="example.com",method="GET",url="/test"} 1`)
assert.Contains(t, body, `echo_request_duration_seconds_count{code="404",host="example.com",method="GET",url="/ping"} 1`)
assert.Contains(t, body, `echo_request_duration_seconds_count{code="404",host="example.com",method="GET",url="/ping"} 1`)
assert.Contains(t, body, `echo_request_duration_seconds_count{code="404",host="example.com",method="GET",url=""} 1`)
assert.Contains(t, body, `echo_request_duration_seconds_count{code="404",host="example.com",method="GET",url=""} 1`)
assert.NotContains(t, body, `test_ignore`) // because we skipped
}

Expand Down Expand Up @@ -284,7 +284,7 @@ func TestRunPushGatewayGatherer(t *testing.T) {
func TestSetPathFor404NoMatchingRoute(t *testing.T) {
e := echo.New()

e.Use(NewMiddlewareWithConfig(MiddlewareConfig{DoNotUseRequestPathFor404: true, Subsystem: defaultSubsystem}))
e.Use(NewMiddlewareWithConfig(MiddlewareConfig{Subsystem: defaultSubsystem}))
e.GET("/metrics", NewHandler())

assert.Equal(t, http.StatusNotFound, request(e, "/nonExistentPath"))
Expand All @@ -297,12 +297,30 @@ func TestSetPathFor404NoMatchingRoute(t *testing.T) {
unregisterDefaults(defaultSubsystem)
}

// TestUseRequestPathFor404 tests that the url is included in the metric when the 404 response is due to no matching
// route and UseRequestPathFor404 is enabled
func TestUseRequestPathFor404(t *testing.T) {
e := echo.New()

e.Use(NewMiddlewareWithConfig(MiddlewareConfig{UseRequestPathFor404: true, Subsystem: defaultSubsystem}))
e.GET("/metrics", NewHandler())

assert.Equal(t, http.StatusNotFound, request(e, "/nonExistentPath"))

s, code := requestBody(e, "/metrics")
assert.Equal(t, http.StatusOK, code)
assert.Contains(t, s, fmt.Sprintf(`%s_request_duration_seconds_count{code="404",host="example.com",method="GET",url="/nonExistentPath"} 1`, defaultSubsystem))
assert.NotContains(t, s, fmt.Sprintf(`%s_request_duration_seconds_count{code="404",host="example.com",method="GET",url=""} 1`, defaultSubsystem))

unregisterDefaults(defaultSubsystem)
}

// TestSetPathFor404Logic tests that the url is included in the metric when the 404 response is due to logic
func TestSetPathFor404Logic(t *testing.T) {
unregisterDefaults("myapp")
e := echo.New()

e.Use(NewMiddlewareWithConfig(MiddlewareConfig{DoNotUseRequestPathFor404: true, Subsystem: defaultSubsystem}))
e.Use(NewMiddlewareWithConfig(MiddlewareConfig{Subsystem: defaultSubsystem}))
e.GET("/metrics", NewHandler())

e.GET("/sample", func(c *echo.Context) error {
Expand All @@ -322,7 +340,7 @@ func TestSetPathFor404Logic(t *testing.T) {
func TestInvalidUTF8PathIsFixed(t *testing.T) {
e := echo.New()

e.Use(NewMiddlewareWithConfig(MiddlewareConfig{Subsystem: defaultSubsystem}))
e.Use(NewMiddlewareWithConfig(MiddlewareConfig{UseRequestPathFor404: true, Subsystem: defaultSubsystem}))
e.GET("/metrics", NewHandler())

assert.Equal(t, http.StatusNotFound, request(e, "/../../WEB-INF/web.xml\xc0\x80.jsp"))
Expand Down