From 3292e4da065d4ab9f83ea388c135c28bfc82eac9 Mon Sep 17 00:00:00 2001 From: Ilham Syahid S Date: Fri, 10 May 2024 23:21:02 +0700 Subject: [PATCH 1/4] feat: add multiple filters in WithFilter option Signed-off-by: Ilham Syahid S --- config.go | 11 ++++++++--- middleware.go | 14 ++++++++------ 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/config.go b/config.go index 3529268..13a8402 100644 --- a/config.go +++ b/config.go @@ -16,7 +16,7 @@ type config struct { Propagators propagation.TextMapPropagator ChiRoutes chi.Routes RequestMethodInSpanName bool - Filter func(r *http.Request) bool + Filters []Filter TraceResponseHeaderKey string PublicEndpointFn func(r *http.Request) bool } @@ -32,6 +32,10 @@ func (o optionFunc) apply(c *config) { o(c) } +// Filter is a predicate used to determine whether a given http.request should +// be traced. +type Filter func(*http.Request) bool + // WithPropagators specifies propagators to use for extracting // information from the HTTP requests. If none are specified, global // ones will be used. @@ -79,9 +83,10 @@ func WithRequestMethodInSpanName(isActive bool) Option { // WithFilter is used for filtering request that should not be traced. // This is useful for filtering health check request, etc. // A Filter must return true if the request should be traced. -func WithFilter(filter func(r *http.Request) bool) Option { +// If no filters are provided then all requests are traced. +func WithFilter(filter Filter) Option { return optionFunc(func(cfg *config) { - cfg.Filter = filter + cfg.Filters = append(cfg.Filters, filter) }) } diff --git a/middleware.go b/middleware.go index d6dfda0..006dc9e 100644 --- a/middleware.go +++ b/middleware.go @@ -46,7 +46,7 @@ func Middleware(serverName string, opts ...Option) func(next http.Handler) http. handler: handler, chiRoutes: cfg.ChiRoutes, reqMethodInSpanName: cfg.RequestMethodInSpanName, - filter: cfg.Filter, + filters: cfg.Filters, traceResponseHeaderKey: cfg.TraceResponseHeaderKey, publicEndpointFn: cfg.PublicEndpointFn, } @@ -60,7 +60,7 @@ type traceware struct { handler http.Handler chiRoutes chi.Routes reqMethodInSpanName bool - filter func(r *http.Request) bool + filters []Filter traceResponseHeaderKey string publicEndpointFn func(r *http.Request) bool } @@ -111,10 +111,12 @@ func putRRW(rrw *recordingResponseWriter) { // ServeHTTP implements the http.Handler interface. It does the actual // tracing of the request. func (tw traceware) ServeHTTP(w http.ResponseWriter, r *http.Request) { - // skip if filter returns false - if tw.filter != nil && !tw.filter(r) { - tw.handler.ServeHTTP(w, r) - return + for _, filter := range tw.filters { + // simply pass through to the handler if filter returns false + if !filter(r) { + tw.handler.ServeHTTP(w, r) + return + } } // extract tracing header using propagator From 9cd52ca9fbe615015643aeedf9fccc712db8f1b4 Mon Sep 17 00:00:00 2001 From: Ilham Syahid S Date: Fri, 10 May 2024 23:21:59 +0700 Subject: [PATCH 2/4] test: adjust WithFilter test for multiple filters Signed-off-by: Ilham Syahid S --- test/cases/sdk_test.go | 146 +++++++++++++++++++++++++++-------------- 1 file changed, 97 insertions(+), 49 deletions(-) diff --git a/test/cases/sdk_test.go b/test/cases/sdk_test.go index 1e75f55..df9b1ab 100644 --- a/test/cases/sdk_test.go +++ b/test/cases/sdk_test.go @@ -63,59 +63,107 @@ func TestSDKIntegration(t *testing.T) { }) } -func TestSDKIntegrationWithFilters(t *testing.T) { - // prepare router and span recorder - router, sr := newSDKTestRouter("foobar", false, otelchi.WithFilter(func(r *http.Request) bool { - // if client access /live or /ready, there should be no span - if r.URL.Path == "/live" || r.URL.Path == "/ready" { - return false - } - - // otherwise always return the span - return true - })) - - // define router - router.HandleFunc("/user/{id:[0-9]+}", ok) - router.HandleFunc("/book/{title}", ok) - router.HandleFunc("/health", ok) - router.HandleFunc("/ready", ok) - - // execute requests - executeRequests(router, []*http.Request{ - httptest.NewRequest("GET", "/user/123", nil), - httptest.NewRequest("GET", "/book/foo", nil), - httptest.NewRequest("GET", "/live", nil), - httptest.NewRequest("GET", "/ready", nil), - }) - - // get recorded spans and ensure the length is 2 - recordedSpans := sr.Ended() - require.Len(t, recordedSpans, 2) - - // ensure span values - checkSpans(t, recordedSpans, []spanValueCheck{ +func TestSDKIntegrationWithFilter(t *testing.T) { + // prepare test cases + serviceName := "foobar" + testCases := []struct { + Name string + FilterFn []otelchi.Filter + LenSpans int + ExpectedRouteNames []string + }{ { - Name: "/user/{id:[0-9]+}", - Kind: trace.SpanKindServer, - Attributes: getSemanticAttributes( - "foobar", - http.StatusOK, - "GET", - "/user/{id:[0-9]+}", - ), + Name: "One WithFilter", + FilterFn: []otelchi.Filter{ + func(r *http.Request) bool { + return r.URL.Path != "/live" && r.URL.Path != "/ready" + }, + }, + LenSpans: 2, + ExpectedRouteNames: []string{"/user/{id:[0-9]+}", "/book/{title}"}, }, { - Name: "/book/{title}", - Kind: trace.SpanKindServer, - Attributes: getSemanticAttributes( - "foobar", - http.StatusOK, - "GET", - "/book/{title}", - ), + Name: "Multiple WithFilter", + FilterFn: []otelchi.Filter{ + func(r *http.Request) bool { + return r.URL.Path != "/ready" + }, + func(r *http.Request) bool { + return r.URL.Path != "/live" + }, + }, + LenSpans: 2, + ExpectedRouteNames: []string{"/user/{id:[0-9]+}", "/book/{title}"}, }, - }) + { + Name: "All Routes are traced", + FilterFn: []otelchi.Filter{ + func(r *http.Request) bool { + return true + }, + }, + LenSpans: 4, + ExpectedRouteNames: []string{"/user/{id:[0-9]+}", "/book/{title}", "/live", "/ready"}, + }, + { + Name: "All Routes are not traced", + FilterFn: []otelchi.Filter{ + func(r *http.Request) bool { + return false + }, + }, + LenSpans: 0, + ExpectedRouteNames: []string{}, + }, + } + + // execute test cases + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + // prepare router and span recorder + filters := []otelchi.Option{} + for _, filter := range testCase.FilterFn { + filters = append(filters, otelchi.WithFilter(filter)) + } + router, sr := newSDKTestRouter(serviceName, false, filters...) + + // define router + router.HandleFunc("/user/{id:[0-9]+}", ok) + router.HandleFunc("/book/{title}", ok) + router.HandleFunc("/health", ok) + router.HandleFunc("/live", ok) + router.HandleFunc("/ready", ok) + + // execute requests + executeRequests(router, []*http.Request{ + httptest.NewRequest("GET", "/user/123", nil), + httptest.NewRequest("GET", "/book/foo", nil), + httptest.NewRequest("GET", "/live", nil), + httptest.NewRequest("GET", "/ready", nil), + }) + + // check recorded spans + recordedSpans := sr.Ended() + require.Len(t, recordedSpans, testCase.LenSpans) + + // ensure span values + spanValues := []spanValueCheck{} + for _, routeName := range testCase.ExpectedRouteNames { + spanValues = append(spanValues, spanValueCheck{ + Name: routeName, + Kind: trace.SpanKindServer, + Attributes: getSemanticAttributes( + serviceName, + http.StatusOK, + "GET", + routeName, + ), + }) + } + checkSpans(t, recordedSpans, spanValues) + }) + } + } func TestSDKIntegrationWithChiRoutes(t *testing.T) { From 61347e603afeb70d4f4adc50d9bf0753596ddeae Mon Sep 17 00:00:00 2001 From: Ilham Syahid S Date: Fri, 10 May 2024 23:31:50 +0700 Subject: [PATCH 3/4] docs: add `WithFilter` adjustment on CHANGELOG Signed-off-by: Ilham Syahid S --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d5aa47..ad67137 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Changed + +- Adjust `WithFilter` option to accept multiple functions. ([#47]) + ## [0.8.0] - 2024-04-29 ### ⚠️ Notice ⚠️ @@ -161,6 +165,7 @@ It contains instrumentation for trace and depends on: - Example code for a basic usage. - Apache-2.0 license. +[#47]: https://github.com/riandyrn/otelchi/pull/47 [#43]: https://github.com/riandyrn/otelchi/pull/43 [#42]: https://github.com/riandyrn/otelchi/pull/42 [#41]: https://github.com/riandyrn/otelchi/pull/41 From 2cd6aaffa742055f1cbe9254df73911eebcc1e5d Mon Sep 17 00:00:00 2001 From: Riandy Rahman Nugraha Date: Sat, 6 Jul 2024 14:57:55 +0700 Subject: [PATCH 4/4] docs: update comments related to WithFilters to make it easier to understand; --- CHANGELOG.md | 2 +- config.go | 10 ++++++---- middleware.go | 4 +++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad67137..ea94ff0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed -- Adjust `WithFilter` option to accept multiple functions. ([#47]) +- `WithFilter` option now support multiple filter functions, just like in [otelmux](https://github.com/open-telemetry/opentelemetry-go-contrib/blob/v1.24.0/instrumentation/github.com/gorilla/mux/otelmux/config.go#L106-L110). ([#47]) ## [0.8.0] - 2024-04-29 diff --git a/config.go b/config.go index 13a8402..1763a28 100644 --- a/config.go +++ b/config.go @@ -33,7 +33,7 @@ func (o optionFunc) apply(c *config) { } // Filter is a predicate used to determine whether a given http.request should -// be traced. +// be traced. A Filter must return true if the request should be traced. type Filter func(*http.Request) bool // WithPropagators specifies propagators to use for extracting @@ -80,10 +80,12 @@ func WithRequestMethodInSpanName(isActive bool) Option { }) } -// WithFilter is used for filtering request that should not be traced. -// This is useful for filtering health check request, etc. -// A Filter must return true if the request should be traced. +// WithFilter adds a filter to the list of filters used by the handler. +// If any filter indicates to exclude a request then the request will not be +// traced. All filters must allow a request to be traced for a Span to be created. // If no filters are provided then all requests are traced. +// Filters will be invoked for each processed request, it is advised to make them +// simple and fast. func WithFilter(filter Filter) Option { return optionFunc(func(cfg *config) { cfg.Filters = append(cfg.Filters, filter) diff --git a/middleware.go b/middleware.go index 006dc9e..4d9cdaf 100644 --- a/middleware.go +++ b/middleware.go @@ -111,8 +111,10 @@ func putRRW(rrw *recordingResponseWriter) { // ServeHTTP implements the http.Handler interface. It does the actual // tracing of the request. func (tw traceware) ServeHTTP(w http.ResponseWriter, r *http.Request) { + // go through all filters if any for _, filter := range tw.filters { - // simply pass through to the handler if filter returns false + // if there is a filter that returns false, we skip tracing + // and execute next handler if !filter(r) { tw.handler.ServeHTTP(w, r) return