From e08ed89403255de53db87c60e63c6b55a0f21c02 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Wed, 16 Nov 2022 13:52:06 -0700 Subject: [PATCH] Polish Span and Meter Names Closes gh-12156 --- .../HttpSecurityObservationTests.java | 4 +- .../security/config/http/HttpConfigTests.java | 4 +- .../AuthenticationObservationConvention.java | 15 ++++ .../AuthorizationObservationConvention.java | 33 +++++++-- ...rvationSecurityContextChangedListener.java | 6 +- .../web/ObservationFilterChainDecorator.java | 68 +++++++------------ .../ObservationWebFilterChainDecorator.java | 58 ++++++---------- 7 files changed, 94 insertions(+), 94 deletions(-) diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecurityObservationTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecurityObservationTests.java index 7cc85c7967..2bb4e9684d 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecurityObservationTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecurityObservationTests.java @@ -72,11 +72,11 @@ public class HttpSecurityObservationTests { ArgumentCaptor captor = ArgumentCaptor.forClass(Observation.Context.class); verify(handler, times(5)).onStart(captor.capture()); Iterator contexts = captor.getAllValues().iterator(); - assertThat(contexts.next().getContextualName()).isEqualTo("spring.security.http.chains.before"); + assertThat(contexts.next().getContextualName()).isEqualTo("security filterchain before"); assertThat(contexts.next().getName()).isEqualTo("spring.security.authentications"); assertThat(contexts.next().getName()).isEqualTo("spring.security.authorizations"); assertThat(contexts.next().getName()).isEqualTo("spring.security.http.secured.requests"); - assertThat(contexts.next().getContextualName()).isEqualTo("spring.security.http.chains.after"); + assertThat(contexts.next().getContextualName()).isEqualTo("security filterchain after"); } @EnableWebSecurity diff --git a/config/src/test/java/org/springframework/security/config/http/HttpConfigTests.java b/config/src/test/java/org/springframework/security/config/http/HttpConfigTests.java index d398366d62..4a6ed181a3 100644 --- a/config/src/test/java/org/springframework/security/config/http/HttpConfigTests.java +++ b/config/src/test/java/org/springframework/security/config/http/HttpConfigTests.java @@ -122,11 +122,11 @@ public class HttpConfigTests { ArgumentCaptor captor = ArgumentCaptor.forClass(Observation.Context.class); verify(handler, times(5)).onStart(captor.capture()); Iterator contexts = captor.getAllValues().iterator(); - assertThat(contexts.next().getContextualName()).isEqualTo("spring.security.http.chains.before"); + assertThat(contexts.next().getContextualName()).isEqualTo("security filterchain before"); assertThat(contexts.next().getName()).isEqualTo("spring.security.authentications"); assertThat(contexts.next().getName()).isEqualTo("spring.security.authorizations"); assertThat(contexts.next().getName()).isEqualTo("spring.security.http.secured.requests"); - assertThat(contexts.next().getContextualName()).isEqualTo("spring.security.http.chains.after"); + assertThat(contexts.next().getContextualName()).isEqualTo("security filterchain after"); } private String xml(String configName) { diff --git a/core/src/main/java/org/springframework/security/authentication/AuthenticationObservationConvention.java b/core/src/main/java/org/springframework/security/authentication/AuthenticationObservationConvention.java index 0d229ad05b..b968fc04da 100644 --- a/core/src/main/java/org/springframework/security/authentication/AuthenticationObservationConvention.java +++ b/core/src/main/java/org/springframework/security/authentication/AuthenticationObservationConvention.java @@ -43,6 +43,21 @@ public final class AuthenticationObservationConvention return OBSERVATION_NAME; } + @Override + public String getContextualName(AuthenticationObservationContext context) { + if (context.getAuthenticationRequest() != null) { + String authenticationType = context.getAuthenticationRequest().getClass().getSimpleName(); + if (authenticationType.endsWith("Token")) { + authenticationType = authenticationType.substring(0, authenticationType.lastIndexOf("Token")); + } + if (authenticationType.endsWith("Authentication")) { + authenticationType = authenticationType.substring(0, authenticationType.lastIndexOf("Authentication")); + } + return "authenticate " + authenticationType.toLowerCase(); + } + return "authenticate"; + } + /** * {@inheritDoc} */ diff --git a/core/src/main/java/org/springframework/security/authorization/AuthorizationObservationConvention.java b/core/src/main/java/org/springframework/security/authorization/AuthorizationObservationConvention.java index 8e2f6f8499..a2fe0ec8b9 100644 --- a/core/src/main/java/org/springframework/security/authorization/AuthorizationObservationConvention.java +++ b/core/src/main/java/org/springframework/security/authorization/AuthorizationObservationConvention.java @@ -19,6 +19,9 @@ package org.springframework.security.authorization; import io.micrometer.common.KeyValues; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationConvention; +import org.aopalliance.intercept.MethodInvocation; + +import org.springframework.security.authorization.method.MethodInvocationResult; /** * An {@link ObservationConvention} for translating authorizations into {@link KeyValues}. @@ -39,14 +42,19 @@ public final class AuthorizationObservationConvention return OBSERVATION_NAME; } + @Override + public String getContextualName(AuthorizationObservationContext context) { + return "authorize " + getObjectType(context); + } + /** * {@inheritDoc} */ @Override public KeyValues getLowCardinalityKeyValues(AuthorizationObservationContext context) { - return KeyValues.of("authentication.type", getAuthenticationType(context)) - .and("object.type", getObjectType(context)) - .and("authorization.decision", getAuthorizationDecision(context)); + return KeyValues.of("spring.security.authentication.type", getAuthenticationType(context)) + .and("spring.security.object", getObjectType(context)) + .and("spring.security.authorization.decision", getAuthorizationDecision(context)); } /** @@ -54,8 +62,8 @@ public final class AuthorizationObservationConvention */ @Override public KeyValues getHighCardinalityKeyValues(AuthorizationObservationContext context) { - return KeyValues.of("authentication.authorities", getAuthorities(context)).and("authorization.decision.details", - getDecisionDetails(context)); + return KeyValues.of("spring.security.authentication.authorities", getAuthorities(context)) + .and("spring.security.authorization.decision.details", getDecisionDetails(context)); } @Override @@ -74,7 +82,20 @@ public final class AuthorizationObservationConvention if (context.getObject() == null) { return "unknown"; } - return context.getObject().getClass().getSimpleName(); + if (context.getObject() instanceof MethodInvocation) { + return "method"; + } + if (context.getObject() instanceof MethodInvocationResult) { + return "method"; + } + String className = context.getObject().getClass().getSimpleName(); + if (className.contains("Request")) { + return "request"; + } + if (className.contains("Message")) { + return "message"; + } + return className; } private String getAuthorizationDecision(AuthorizationObservationContext context) { diff --git a/core/src/main/java/org/springframework/security/core/context/ObservationSecurityContextChangedListener.java b/core/src/main/java/org/springframework/security/core/context/ObservationSecurityContextChangedListener.java index ef582c1fbc..63e300818a 100644 --- a/core/src/main/java/org/springframework/security/core/context/ObservationSecurityContextChangedListener.java +++ b/core/src/main/java/org/springframework/security/core/context/ObservationSecurityContextChangedListener.java @@ -33,11 +33,11 @@ import org.springframework.security.core.Authentication; */ public final class ObservationSecurityContextChangedListener implements SecurityContextChangedListener { - static final String SECURITY_CONTEXT_CREATED = "security.context.created"; + static final String SECURITY_CONTEXT_CREATED = "spring.security.context.created"; - static final String SECURITY_CONTEXT_CHANGED = "security.context.changed"; + static final String SECURITY_CONTEXT_CHANGED = "spring.security.context.changed"; - static final String SECURITY_CONTEXT_CLEARED = "security.context.cleared"; + static final String SECURITY_CONTEXT_CLEARED = "spring.security.context.cleared"; private final ObservationRegistry registry; diff --git a/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java b/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java index 8f50c52a11..f61571e566 100644 --- a/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java +++ b/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java @@ -37,11 +37,10 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.core.log.LogMessage; -import org.springframework.security.web.util.UrlUtils; /** - * A {@link org.springframework.security.web.server.FilterChainProxy.FilterChainDecorator} - * that wraps the chain in before and after observations + * A {@link org.springframework.security.web.FilterChainProxy.FilterChainDecorator} that + * wraps the chain in before and after observations * * @author Josh Cummings * @since 6.0 @@ -75,14 +74,16 @@ public final class ObservationFilterChainDecorator implements FilterChainProxy.F private FilterChain wrapSecured(FilterChain original) { return (req, res) -> { AroundFilterObservation parent = observation((HttpServletRequest) req); - Observation observation = Observation.createNotStarted(SECURED_OBSERVATION_NAME, this.registry); + Observation observation = Observation.createNotStarted(SECURED_OBSERVATION_NAME, this.registry) + .contextualName("secured request"); parent.wrap(FilterObservation.create(observation).wrap(original)).doFilter(req, res); }; } private FilterChain wrapUnsecured(FilterChain original) { return (req, res) -> { - Observation observation = Observation.createNotStarted(UNSECURED_OBSERVATION_NAME, this.registry); + Observation observation = Observation.createNotStarted(UNSECURED_OBSERVATION_NAME, this.registry) + .contextualName("unsecured request"); FilterObservation.create(observation).wrap(original).doFilter(req, res); }; } @@ -189,8 +190,8 @@ public final class ObservationFilterChainDecorator implements FilterChainProxy.F } private AroundFilterObservation parent(HttpServletRequest request) { - FilterChainObservationContext beforeContext = FilterChainObservationContext.before(request); - FilterChainObservationContext afterContext = FilterChainObservationContext.after(request); + FilterChainObservationContext beforeContext = FilterChainObservationContext.before(); + FilterChainObservationContext afterContext = FilterChainObservationContext.after(); Observation before = Observation.createNotStarted(this.convention, () -> beforeContext, this.registry); Observation after = Observation.createNotStarted(this.convention, () -> afterContext, this.registry); AroundFilterObservation parent = AroundFilterObservation.create(before, after); @@ -409,8 +410,6 @@ public final class ObservationFilterChainDecorator implements FilterChainProxy.F static final class FilterChainObservationContext extends Observation.Context { - private final ServletRequest request; - private final String filterSection; private String filterName; @@ -419,29 +418,17 @@ public final class ObservationFilterChainDecorator implements FilterChainProxy.F private int chainSize; - private FilterChainObservationContext(ServletRequest request, String filterSection) { + private FilterChainObservationContext(String filterSection) { this.filterSection = filterSection; - this.request = request; + setContextualName("security filterchain " + filterSection); } - static FilterChainObservationContext before(ServletRequest request) { - return new FilterChainObservationContext(request, "before"); + static FilterChainObservationContext before() { + return new FilterChainObservationContext("before"); } - static FilterChainObservationContext after(ServletRequest request) { - return new FilterChainObservationContext(request, "after"); - } - - @Override - public void setName(String name) { - super.setName(name); - if (name != null) { - setContextualName(name + "." + this.filterSection); - } - } - - String getRequestLine() { - return requestLine((HttpServletRequest) this.request); + static FilterChainObservationContext after() { + return new FilterChainObservationContext("after"); } String getFilterSection() { @@ -472,32 +459,31 @@ public final class ObservationFilterChainDecorator implements FilterChainProxy.F this.chainSize = chainSize; } - private static String requestLine(HttpServletRequest request) { - return request.getMethod() + " " + UrlUtils.buildRequestUrl(request); - } - } static final class FilterChainObservationConvention implements ObservationConvention { - static final String CHAIN_OBSERVATION_NAME = "spring.security.http.chains"; + static final String CHAIN_OBSERVATION_NAME = "spring.security.filterchains"; - private static final String REQUEST_LINE_NAME = "request.line"; + private static final String CHAIN_POSITION_NAME = "spring.security.filterchain.position"; - private static final String CHAIN_POSITION_NAME = "chain.position"; + private static final String CHAIN_SIZE_NAME = "spring.security.filterchain.size"; - private static final String CHAIN_SIZE_NAME = "chain.size"; + private static final String FILTER_SECTION_NAME = "security.security.reached.filter.section"; - private static final String FILTER_SECTION_NAME = "filter.section"; - - private static final String FILTER_NAME = "current.filter.name"; + private static final String FILTER_NAME = "spring.security.reached.filter.name"; @Override public String getName() { return CHAIN_OBSERVATION_NAME; } + @Override + public String getContextualName(FilterChainObservationContext context) { + return "security filterchain " + context.getFilterSection(); + } + @Override public KeyValues getLowCardinalityKeyValues(FilterChainObservationContext context) { KeyValues kv = KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) @@ -509,12 +495,6 @@ public final class ObservationFilterChainDecorator implements FilterChainProxy.F return kv; } - @Override - public KeyValues getHighCardinalityKeyValues(FilterChainObservationContext context) { - String requestLine = context.getRequestLine(); - return KeyValues.of(REQUEST_LINE_NAME, requestLine); - } - @Override public boolean supportsContext(Observation.Context context) { return context instanceof FilterChainObservationContext; diff --git a/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java b/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java index c815dae5a0..efd4482a9c 100644 --- a/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java +++ b/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java @@ -75,14 +75,16 @@ public final class ObservationWebFilterChainDecorator implements WebFilterChainP private WebFilterChain wrapSecured(WebFilterChain original) { return (exchange) -> { AroundWebFilterObservation parent = observation(exchange); - Observation observation = Observation.createNotStarted(SECURED_OBSERVATION_NAME, this.registry); + Observation observation = Observation.createNotStarted(SECURED_OBSERVATION_NAME, this.registry) + .contextualName("secured request"); return parent.wrap(WebFilterObservation.create(observation).wrap(original)).filter(exchange); }; } private WebFilterChain wrapUnsecured(WebFilterChain original) { return (exchange) -> { - Observation observation = Observation.createNotStarted(UNSECURED_OBSERVATION_NAME, this.registry); + Observation observation = Observation.createNotStarted(UNSECURED_OBSERVATION_NAME, this.registry) + .contextualName("unsecured request"); return WebFilterObservation.create(observation).wrap(original).filter(exchange); }; } @@ -210,8 +212,8 @@ public final class ObservationWebFilterChainDecorator implements WebFilterChainP } private AroundWebFilterObservation parent(ServerWebExchange exchange) { - WebFilterChainObservationContext beforeContext = WebFilterChainObservationContext.before(exchange); - WebFilterChainObservationContext afterContext = WebFilterChainObservationContext.after(exchange); + WebFilterChainObservationContext beforeContext = WebFilterChainObservationContext.before(); + WebFilterChainObservationContext afterContext = WebFilterChainObservationContext.after(); Observation before = Observation.createNotStarted(this.convention, () -> beforeContext, this.registry); Observation after = Observation.createNotStarted(this.convention, () -> afterContext, this.registry); AroundWebFilterObservation parent = AroundWebFilterObservation.create(before, after); @@ -419,8 +421,6 @@ public final class ObservationWebFilterChainDecorator implements WebFilterChainP static final class WebFilterChainObservationContext extends Observation.Context { - private final ServerWebExchange exchange; - private final String filterSection; private String filterName; @@ -429,29 +429,16 @@ public final class ObservationWebFilterChainDecorator implements WebFilterChainP private int chainSize; - private WebFilterChainObservationContext(ServerWebExchange exchange, String filterSection) { - this.exchange = exchange; + private WebFilterChainObservationContext(String filterSection) { this.filterSection = filterSection; } - static WebFilterChainObservationContext before(ServerWebExchange exchange) { - return new WebFilterChainObservationContext(exchange, "before"); + static WebFilterChainObservationContext before() { + return new WebFilterChainObservationContext("before"); } - static WebFilterChainObservationContext after(ServerWebExchange exchange) { - return new WebFilterChainObservationContext(exchange, "after"); - } - - @Override - public void setName(String name) { - super.setName(name); - if (name != null) { - setContextualName(name + "." + this.filterSection); - } - } - - String getRequestLine() { - return this.exchange.getRequest().getPath().toString(); + static WebFilterChainObservationContext after() { + return new WebFilterChainObservationContext("after"); } String getFilterSection() { @@ -487,23 +474,26 @@ public final class ObservationWebFilterChainDecorator implements WebFilterChainP static final class WebFilterChainObservationConvention implements ObservationConvention { - static final String CHAIN_OBSERVATION_NAME = "spring.security.http.chains"; + static final String CHAIN_OBSERVATION_NAME = "spring.security.filterchains"; - private static final String REQUEST_LINE_NAME = "request.line"; + private static final String CHAIN_POSITION_NAME = "spring.security.filterchain.position"; - private static final String CHAIN_POSITION_NAME = "chain.position"; + private static final String CHAIN_SIZE_NAME = "spring.security.filterchain.size"; - private static final String CHAIN_SIZE_NAME = "chain.size"; + private static final String FILTER_SECTION_NAME = "spring.security.reached.filter.section"; - private static final String FILTER_SECTION_NAME = "filter.section"; - - private static final String FILTER_NAME = "current.filter.name"; + private static final String FILTER_NAME = "spring.security.reached.filter.name"; @Override public String getName() { return CHAIN_OBSERVATION_NAME; } + @Override + public String getContextualName(WebFilterChainObservationContext context) { + return "security filterchain " + context.getFilterSection(); + } + @Override public KeyValues getLowCardinalityKeyValues(WebFilterChainObservationContext context) { KeyValues kv = KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) @@ -515,12 +505,6 @@ public final class ObservationWebFilterChainDecorator implements WebFilterChainP return kv; } - @Override - public KeyValues getHighCardinalityKeyValues(WebFilterChainObservationContext context) { - String requestLine = context.getRequestLine(); - return KeyValues.of(REQUEST_LINE_NAME, requestLine); - } - @Override public boolean supportsContext(Observation.Context context) { return context instanceof WebFilterChainObservationContext;