Use consistent list of micrometer tags in web observation handler

The tag `spring.security.reached.filter.name` is only set if a
filter-name is available, otherwise the tag is omitted entirely. This
leads to issues with metric-exporters that don't support dynamic tags,
but rather expect tag-names of a metric to be always the same. The most
prominent example is the Prometheus-exporter.

Instead of omitting the tag if no filer-name is set, a none-value is
applied instead, making the tag-list consistent in all cases

Closes gh-13179

Author: dfrommi

web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java

@@ -22,6 +22,7 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
+import io.micrometer.common.KeyValue;
 import io.micrometer.common.KeyValues;
 import io.micrometer.observation.Observation;
 import io.micrometer.observation.ObservationConvention;
@@ -515,13 +516,11 @@ public String getContextualName(FilterChainObservationContext context) {
 
 		@Override
 		public KeyValues getLowCardinalityKeyValues(FilterChainObservationContext context) {
-			KeyValues kv = KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize()))
+			return KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize()))
 					.and(CHAIN_POSITION_NAME, String.valueOf(context.getChainPosition()))
-					.and(FILTER_SECTION_NAME, context.getFilterSection());
-			if (context.getFilterName() != null) {
-				kv = kv.and(FILTER_NAME, context.getFilterName());
-			}
-			return kv;
+					.and(FILTER_SECTION_NAME, context.getFilterSection())
+					.and(FILTER_NAME, (context.getFilterName() != null && !context.getFilterName().isEmpty())
+							? context.getFilterName() : KeyValue.NONE_VALUE);
 		}
 
 		@Override

web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java

@@ -686,13 +686,11 @@ public String getContextualName(WebFilterChainObservationContext context) {
 
 		@Override
 		public KeyValues getLowCardinalityKeyValues(WebFilterChainObservationContext context) {
-			KeyValues kv = KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize()))
+			return KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize()))
 					.and(CHAIN_POSITION_NAME, String.valueOf(context.getChainPosition()))
-					.and(FILTER_SECTION_NAME, context.getFilterSection());
-			if (context.getFilterName() != null) {
-				kv = kv.and(FILTER_NAME, context.getFilterName());
-			}
-			return kv;
+					.and(FILTER_SECTION_NAME, context.getFilterSection())
+					.and(FILTER_NAME, (context.getFilterName() != null && !context.getFilterName().isEmpty())
+							? context.getFilterName() : KeyValue.NONE_VALUE);
 		}
 
 		@Override

web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java

@@ -16,14 +16,22 @@
 
 package org.springframework.security.web;
 
+import java.io.IOException;
 import java.util.List;
+import java.util.stream.Stream;
 
 import io.micrometer.observation.Observation;
 import io.micrometer.observation.ObservationHandler;
 import io.micrometer.observation.ObservationRegistry;
 import jakarta.servlet.Filter;
 import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.ArgumentCaptor;
 
 import org.springframework.mock.web.MockHttpServletRequest;
@@ -106,4 +114,48 @@ void decorateFiltersWhenErrorsThenClosesObservationOnlyOnce() throws Exception {
 		verify(handler).onScopeClosed(any());
 	}
 
+	@ParameterizedTest
+	@MethodSource("decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag")
+	void decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag(Filter filter,
+			String expectedFilterNameTag) throws Exception {
+		ObservationHandler<Observation.Context> handler = mock(ObservationHandler.class);
+		given(handler.supportsContext(any())).willReturn(true);
+		ObservationRegistry registry = ObservationRegistry.create();
+		registry.observationConfig().observationHandler(handler);
+		ObservationFilterChainDecorator decorator = new ObservationFilterChainDecorator(registry);
+		FilterChain chain = mock(FilterChain.class);
+		FilterChain decorated = decorator.decorate(chain, List.of(filter));
+		decorated.doFilter(new MockHttpServletRequest("GET", "/"), new MockHttpServletResponse());
+		ArgumentCaptor<Observation.Context> context = ArgumentCaptor.forClass(Observation.Context.class);
+		verify(handler, times(3)).onScopeClosed(context.capture());
+		assertThat(context.getValue().getLowCardinalityKeyValue("spring.security.reached.filter.name").getValue())
+				.isEqualTo(expectedFilterNameTag);
+	}
+
+	static Stream<Arguments> decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag() {
+		Filter filterWithName = new BasicAuthenticationFilter();
+
+		// Anonymous class leads to an empty filter-name
+		Filter filterWithoutName = new Filter() {
+			@Override
+			public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
+					throws IOException, ServletException {
+				chain.doFilter(request, response);
+			}
+		};
+
+		return Stream.of(Arguments.of(filterWithName, "BasicAuthenticationFilter"),
+				Arguments.of(filterWithoutName, "none"));
+	}
+
+	private static class BasicAuthenticationFilter implements Filter {
+
+		@Override
+		public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
+				throws IOException, ServletException {
+			chain.doFilter(request, response);
+		}
+
+	}
+
 }

web/src/test/java/org/springframework/security/web/server/ObservationWebFilterChainDecoratorTests.java

@@ -18,23 +18,30 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.stream.Stream;
 
 import io.micrometer.observation.Observation;
 import io.micrometer.observation.ObservationHandler;
 import io.micrometer.observation.ObservationRegistry;
 import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.ArgumentCaptor;
 import reactor.core.publisher.Mono;
 
 import org.springframework.mock.http.server.reactive.MockServerHttpRequest;
 import org.springframework.mock.web.server.MockServerWebExchange;
+import org.springframework.web.server.ServerWebExchange;
 import org.springframework.web.server.WebFilter;
 import org.springframework.web.server.WebFilterChain;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
 
@@ -113,6 +120,51 @@ void decorateWhenCustomAfterFilterThenObserves() {
 		handler.assertSpanStop(9, "http");
 	}
 
+	@ParameterizedTest
+	@MethodSource("decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTagArguments")
+	void decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag(WebFilter filter,
+			String expectedFilterNameTag) {
+		ObservationHandler<Observation.Context> handler = mock(ObservationHandler.class);
+		given(handler.supportsContext(any())).willReturn(true);
+		ObservationRegistry registry = ObservationRegistry.create();
+		registry.observationConfig().observationHandler(handler);
+		ObservationWebFilterChainDecorator decorator = new ObservationWebFilterChainDecorator(registry);
+		WebFilterChain chain = mock(WebFilterChain.class);
+		given(chain.filter(any())).willReturn(Mono.empty());
+		WebFilterChain decorated = decorator.decorate(chain, List.of(filter));
+		decorated.filter(MockServerWebExchange.from(MockServerHttpRequest.get("/").build())).block();
+
+		ArgumentCaptor<Observation.Context> context = ArgumentCaptor.forClass(Observation.Context.class);
+		verify(handler, times(3)).onStop(context.capture());
+
+		assertThat(context.getValue().getLowCardinalityKeyValue("spring.security.reached.filter.name").getValue())
+				.isEqualTo(expectedFilterNameTag);
+	}
+
+	static Stream<Arguments> decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTagArguments() {
+		WebFilter filterWithName = new BasicAuthenticationFilter();
+
+		// Anonymous class leads to an empty filter-name
+		WebFilter filterWithoutName = new WebFilter() {
+			@Override
+			public Mono<Void> filter(ServerWebExchange exchange, WebFilterChain chain) {
+				return chain.filter(exchange);
+			}
+		};
+
+		return Stream.of(Arguments.of(filterWithName, "BasicAuthenticationFilter"),
+				Arguments.of(filterWithoutName, "none"));
+	}
+
+	static class BasicAuthenticationFilter implements WebFilter {
+
+		@Override
+		public Mono<Void> filter(ServerWebExchange exchange, WebFilterChain chain) {
+			return chain.filter(exchange);
+		}
+
+	}
+
 	static class AccumulatingObservationHandler implements ObservationHandler<Observation.Context> {
 
 		List<Event> contexts = new ArrayList<>();