From 6397be938e6e75e70fd423411f82ba5ea34052b2 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Thu, 10 Mar 2022 17:15:04 +0100 Subject: [PATCH 01/15] Initial version with TX, Context Propagation and Observation --- .../scheduling/quartz/SchedulerAccessor.java | 29 +++- ...stractPollingMessageListenerContainer.java | 28 +++- spring-tx/spring-tx.gradle | 2 + .../interceptor/TransactionAspectSupport.java | 25 +++- .../DefaultTransactionTagsProvider.java | 96 +++++++++++++ ...ObservationPlatformTransactionManager.java | 129 ++++++++++++++++++ .../support/TransactionObservation.java | 114 ++++++++++++++++ .../TransactionObservationContext.java | 72 ++++++++++ .../TransactionSynchronizationManager.java | 12 +- .../support/TransactionTagsProvider.java | 29 ++++ .../support/TransactionTemplate.java | 34 ++++- .../TransactionThreadLocalAccessor.java | 95 +++++++++++++ ...ter.contextpropagation.ThreadLocalAccessor | 1 + 13 files changed, 644 insertions(+), 22 deletions(-) create mode 100644 spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java create mode 100644 spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java create mode 100644 spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java create mode 100644 spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java create mode 100644 spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java create mode 100644 spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java create mode 100644 spring-tx/src/main/resources/META-INF/services/io.micrometer.contextpropagation.ThreadLocalAccessor diff --git a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java index de60cbb9ca5f..57329988f5d3 100644 --- a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java +++ b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java @@ -21,6 +21,8 @@ import java.util.List; import java.util.Map; +import io.micrometer.core.instrument.observation.Observation; +import io.micrometer.core.instrument.observation.ObservationRegistry; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.quartz.Calendar; @@ -43,6 +45,10 @@ import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.TransactionException; import org.springframework.transaction.TransactionStatus; +import org.springframework.transaction.support.DefaultTransactionTagsProvider; +import org.springframework.transaction.support.ObservationPlatformTransactionManager; +import org.springframework.transaction.support.TransactionObservationContext; +import org.springframework.transaction.support.TransactionTagsProvider; /** * Common base class for accessing a Quartz Scheduler, i.e. for registering jobs, @@ -57,7 +63,7 @@ * @author Stephane Nicoll * @since 2.5.6 */ -public abstract class SchedulerAccessor implements ResourceLoaderAware { +public abstract class SchedulerAccessor implements ResourceLoaderAware, Observation.TagsProviderAware { protected final Log logger = LogFactory.getLog(getClass()); @@ -90,6 +96,10 @@ public abstract class SchedulerAccessor implements ResourceLoaderAware { @Nullable protected ResourceLoader resourceLoader; + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + + private TransactionTagsProvider tagsProvider = new DefaultTransactionTagsProvider(); + /** * Set whether any jobs defined on this SchedulerFactoryBean should overwrite @@ -199,14 +209,25 @@ public void setResourceLoader(ResourceLoader resourceLoader) { this.resourceLoader = resourceLoader; } + public void setObservationRegistry(ObservationRegistry observationRegistry) { + this.observationRegistry = observationRegistry; + } + + @Override + public void setTagsProvider(TransactionTagsProvider tagsProvider) { + this.tagsProvider = tagsProvider; + } /** * Register jobs and triggers (within a transaction, if possible). */ protected void registerJobsAndTriggers() throws SchedulerException { TransactionStatus transactionStatus = null; + TransactionDefinition definition = TransactionDefinition.withDefaults(); + TransactionObservationContext context = new TransactionObservationContext(definition, this.transactionManager); + ObservationPlatformTransactionManager observationPlatformTransactionManager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); if (this.transactionManager != null) { - transactionStatus = this.transactionManager.getTransaction(TransactionDefinition.withDefaults()); + transactionStatus = observationPlatformTransactionManager.getTransaction(definition); } try { @@ -249,7 +270,7 @@ protected void registerJobsAndTriggers() throws SchedulerException { catch (Throwable ex) { if (transactionStatus != null) { try { - this.transactionManager.rollback(transactionStatus); + observationPlatformTransactionManager.rollback(transactionStatus); } catch (TransactionException tex) { logger.error("Job registration exception overridden by rollback exception", ex); @@ -266,7 +287,7 @@ protected void registerJobsAndTriggers() throws SchedulerException { } if (transactionStatus != null) { - this.transactionManager.commit(transactionStatus); + observationPlatformTransactionManager.commit(transactionStatus); } } diff --git a/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java b/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java index 3b43a835bd31..1841821a6bed 100644 --- a/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java +++ b/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java @@ -16,6 +16,8 @@ package org.springframework.jms.listener; +import io.micrometer.core.instrument.observation.Observation; +import io.micrometer.core.instrument.observation.ObservationRegistry; import jakarta.jms.Connection; import jakarta.jms.Destination; import jakarta.jms.JMSException; @@ -32,9 +34,13 @@ import org.springframework.transaction.TransactionException; import org.springframework.transaction.TransactionStatus; import org.springframework.transaction.support.DefaultTransactionDefinition; +import org.springframework.transaction.support.DefaultTransactionTagsProvider; +import org.springframework.transaction.support.ObservationPlatformTransactionManager; import org.springframework.transaction.support.ResourceTransactionManager; +import org.springframework.transaction.support.TransactionObservationContext; import org.springframework.transaction.support.TransactionSynchronizationManager; import org.springframework.transaction.support.TransactionSynchronizationUtils; +import org.springframework.transaction.support.TransactionTagsProvider; import org.springframework.util.Assert; /** @@ -75,7 +81,7 @@ * @see #receiveAndExecute * @see #setTransactionManager */ -public abstract class AbstractPollingMessageListenerContainer extends AbstractMessageListenerContainer { +public abstract class AbstractPollingMessageListenerContainer extends AbstractMessageListenerContainer implements Observation.TagsProviderAware { /** * The default receive timeout: 1000 ms = 1 second. @@ -95,6 +101,10 @@ public abstract class AbstractPollingMessageListenerContainer extends AbstractMe private long receiveTimeout = DEFAULT_RECEIVE_TIMEOUT; + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + + private TransactionTagsProvider tagsProvider = new DefaultTransactionTagsProvider(); + @Override public void setSessionTransacted(boolean sessionTransacted) { @@ -183,6 +193,14 @@ protected long getReceiveTimeout() { return this.receiveTimeout; } + public void setObservationRegistry(ObservationRegistry observationRegistry) { + this.observationRegistry = observationRegistry; + } + + @Override + public void setTagsProvider(TransactionTagsProvider tagsProvider) { + this.tagsProvider = tagsProvider; + } @Override public void initialize() { @@ -238,18 +256,20 @@ protected boolean receiveAndExecute( throws JMSException { if (this.transactionManager != null) { + TransactionObservationContext context = new TransactionObservationContext(this.transactionDefinition, this.transactionManager); + ObservationPlatformTransactionManager observationPlatformTransactionManager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); // Execute receive within transaction. - TransactionStatus status = this.transactionManager.getTransaction(this.transactionDefinition); + TransactionStatus status = observationPlatformTransactionManager.getTransaction(this.transactionDefinition); boolean messageReceived; try { messageReceived = doReceiveAndExecute(invoker, session, consumer, status); } catch (JMSException | RuntimeException | Error ex) { - rollbackOnException(this.transactionManager, status, ex); + rollbackOnException(observationPlatformTransactionManager, status, ex); throw ex; } try { - this.transactionManager.commit(status); + observationPlatformTransactionManager.commit(status); } catch (TransactionException ex) { // Propagate transaction system exceptions as infrastructure problems. diff --git a/spring-tx/spring-tx.gradle b/spring-tx/spring-tx.gradle index 17c9ce146a3d..26e1229333a8 100644 --- a/spring-tx/spring-tx.gradle +++ b/spring-tx/spring-tx.gradle @@ -5,6 +5,7 @@ apply plugin: "kotlin" dependencies { api(project(":spring-beans")) api(project(":spring-core")) + api("io.micrometer:micrometer-core") optional(project(":spring-aop")) optional(project(":spring-context")) // for JCA, @EnableTransactionManagement optional("jakarta.ejb:jakarta.ejb-api") @@ -17,6 +18,7 @@ dependencies { optional("org.jetbrains.kotlin:kotlin-stdlib") optional("org.jetbrains.kotlinx:kotlinx-coroutines-core") optional("org.jetbrains.kotlinx:kotlinx-coroutines-reactor") + optional("io.micrometer:context-propagation") testImplementation(project(":spring-core-test")) testImplementation(testFixtures(project(":spring-beans"))) testImplementation(testFixtures(project(":spring-context"))) diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java index 1dbbdaa6055f..01d9962e73b8 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java @@ -20,6 +20,8 @@ import java.util.Properties; import java.util.concurrent.ConcurrentMap; +import io.micrometer.core.instrument.observation.Observation; +import io.micrometer.core.instrument.observation.ObservationRegistry; import io.vavr.control.Try; import kotlin.coroutines.Continuation; import kotlinx.coroutines.reactive.AwaitKt; @@ -50,6 +52,10 @@ import org.springframework.transaction.TransactionSystemException; import org.springframework.transaction.reactive.TransactionContextManager; import org.springframework.transaction.support.CallbackPreferringPlatformTransactionManager; +import org.springframework.transaction.support.DefaultTransactionTagsProvider; +import org.springframework.transaction.support.ObservationPlatformTransactionManager; +import org.springframework.transaction.support.TransactionObservationContext; +import org.springframework.transaction.support.TransactionTagsProvider; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.ConcurrentReferenceHashMap; @@ -89,7 +95,7 @@ * @see #setTransactionAttributes * @see #setTransactionAttributeSource */ -public abstract class TransactionAspectSupport implements BeanFactoryAware, InitializingBean { +public abstract class TransactionAspectSupport implements BeanFactoryAware, InitializingBean, Observation.TagsProviderAware { // NOTE: This class must not implement Serializable because it serves as base // class for AspectJ aspects (which are not allowed to implement Serializable)! @@ -179,6 +185,10 @@ public static TransactionStatus currentTransactionStatus() throws NoTransactionE @Nullable private BeanFactory beanFactory; + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + + private TransactionTagsProvider tagsProvider = new DefaultTransactionTagsProvider(); + private final ConcurrentMap transactionManagerCache = new ConcurrentReferenceHashMap<>(4); @@ -303,6 +313,15 @@ protected final BeanFactory getBeanFactory() { return this.beanFactory; } + public void setObservationRegistry(ObservationRegistry observationRegistry) { + this.observationRegistry = observationRegistry; + } + + @Override + public void setTagsProvider(TransactionTagsProvider tagsProvider) { + this.tagsProvider = tagsProvider; + } + /** * Check that required properties were set. */ @@ -592,6 +611,10 @@ public String getName() { TransactionStatus status = null; if (txAttr != null) { if (tm != null) { + if (this.transactionManager instanceof PlatformTransactionManager) { + TransactionObservationContext context = new TransactionObservationContext(txAttr, this.transactionManager); + tm = new ObservationPlatformTransactionManager((PlatformTransactionManager) this.transactionManager, this.observationRegistry, context, this.tagsProvider); + } status = tm.getTransaction(txAttr); } else { diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java new file mode 100644 index 000000000000..c33e622ac317 --- /dev/null +++ b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java @@ -0,0 +1,96 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.support; + +import io.micrometer.core.instrument.Tags; +import io.micrometer.core.instrument.observation.Observation; + +import org.springframework.transaction.TransactionDefinition; +import org.springframework.util.ClassUtils; +import org.springframework.util.StringUtils; + +/** + * Default implementation for {@link TransactionTagsProvider}. + * + * @author Marcin Grzejszczak + * @since 6.0.0 + */ +public class DefaultTransactionTagsProvider implements TransactionTagsProvider { + + @Override + public Tags getLowCardinalityTags(TransactionObservationContext context) { + if (!context.getTransactionStatus().isNewTransaction()) { + return Tags.empty(); + } + TransactionDefinition transactionDefinition = context.getTransactionDefinition(); + Tags tags = Tags.empty(); + if (transactionDefinition.getTimeout() > 0) { + tags = tags.and(TransactionObservation.LowCardinalityTags.TIMEOUT.of(String.valueOf(transactionDefinition.getTimeout()))); + } + if (StringUtils.hasText(transactionDefinition.getName())) { + tags = tags.and(TransactionObservation.LowCardinalityTags.NAME.of(transactionDefinition.getName())); + } + return tags.and(TransactionObservation.LowCardinalityTags.TRANSACTION_MANAGER.of(ClassUtils.getQualifiedName(context.getTransactionManagerClass())), + TransactionObservation.LowCardinalityTags.READ_ONLY.of(String.valueOf(transactionDefinition.isReadOnly())), + TransactionObservation.LowCardinalityTags.PROPAGATION_LEVEL.of(propagationLevel(transactionDefinition)), + TransactionObservation.LowCardinalityTags.ISOLATION_LEVEL.of(isolationLevel(transactionDefinition))); + } + + @Override + public boolean supportsContext(Observation.Context context) { + return context instanceof TransactionObservationContext; + } + + + private static String propagationLevel(TransactionDefinition def) { + switch (def.getPropagationBehavior()) { + case 0: + return "PROPAGATION_REQUIRED"; + case 1: + return "PROPAGATION_SUPPORTS"; + case 2: + return "PROPAGATION_MANDATORY"; + case 3: + return "PROPAGATION_REQUIRES_NEW"; + case 4: + return "PROPAGATION_NOT_SUPPORTED"; + case 5: + return "PROPAGATION_NEVER"; + case 6: + return "PROPAGATION_NESTED"; + default: + return String.valueOf(def.getPropagationBehavior()); + } + } + + private static String isolationLevel(TransactionDefinition def) { + switch (def.getIsolationLevel()) { + case -1: + return "ISOLATION_DEFAULT"; + case 1: + return "ISOLATION_READ_UNCOMMITTED"; + case 2: + return "ISOLATION_READ_COMMITTED"; + case 4: + return "ISOLATION_REPEATABLE_READ"; + case 8: + return "ISOLATION_SERIALIZABLE"; + default: + return String.valueOf(def.getIsolationLevel()); + } + } +} diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java new file mode 100644 index 000000000000..38bdd4e0519e --- /dev/null +++ b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java @@ -0,0 +1,129 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.support; + +import io.micrometer.core.instrument.observation.Observation; +import io.micrometer.core.instrument.observation.ObservationRegistry; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; +import org.springframework.transaction.TransactionException; +import org.springframework.transaction.TransactionStatus; + +/** + * An observation representation of a {@link PlatformTransactionManager}. + * + * @author Marcin Grzejszczak + * @since 6.0.0 + */ +public final class ObservationPlatformTransactionManager implements PlatformTransactionManager { + + private static final Log log = LogFactory.getLog(ObservationPlatformTransactionManager.class); + + private final PlatformTransactionManager delegate; + + private final ObservationRegistry observationRegistry; + + private final TransactionObservationContext context; + + private final TransactionTagsProvider transactionTagsProvider; + + private Observation observation; + + private Observation.Scope scope; + + public ObservationPlatformTransactionManager(PlatformTransactionManager delegate, ObservationRegistry observationRegistry, TransactionObservationContext context, TransactionTagsProvider transactionTagsProvider) { + this.delegate = delegate; + this.observationRegistry = observationRegistry; + this.context = context; + this.transactionTagsProvider = transactionTagsProvider; + } + + @Override + public TransactionStatus getTransaction(TransactionDefinition definition) throws TransactionException { + Observation obs = this.observationRegistry.getCurrentObservation(); + if (obs == null) { + obs = TransactionObservation.TX_OBSERVATION.observation(this.observationRegistry, this.context) + .tagsProvider(this.transactionTagsProvider) + .start(); + } + this.observation = obs; + this.scope = this.observation.openScope(); + try { + TransactionStatus transaction = this.delegate.getTransaction(definition); + this.context.setTransactionStatus(transaction); + return transaction; + } + catch (Exception ex) { + this.observation.error(ex).stop(); + throw ex; + } + } + + @Override + public void commit(TransactionStatus status) throws TransactionException { + if (this.observation == null) { + if (log.isDebugEnabled()) { + log.debug("Commit called without calling getTransaction first - this shouldn't happen, sth is wrong"); + } + this.delegate.commit(status); + return; + } + try { + if (log.isDebugEnabled()) { + log.debug("Wrapping commit"); + } + this.delegate.commit(status); + } + catch (Exception ex) { + this.observation.error(ex); + throw ex; + } + finally { + this.scope.close(); + this.observation.stop(); + } + } + + @Override + public void rollback(TransactionStatus status) throws TransactionException { + if (this.observation == null) { + if (log.isDebugEnabled()) { + log.debug("No observation found - this shouldn't happen, sth is wrong"); + } + this.delegate.rollback(status); + return; + } + try { + if (log.isDebugEnabled()) { + log.debug("Wrapping rollback"); + } + this.delegate.rollback(status); + } + catch (Exception ex) { + this.observation.error(ex); + throw ex; + } + finally { + this.scope.close(); + this.observation.stop(); + } + } + +} diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java new file mode 100644 index 000000000000..6e3ae4a23da0 --- /dev/null +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java @@ -0,0 +1,114 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.support; + +import io.micrometer.core.instrument.docs.DocumentedObservation; +import io.micrometer.core.instrument.docs.TagKey; + +enum TransactionObservation implements DocumentedObservation { + + /** + * Observation created when there was no previous transaction. If there was one, we will + * continue it unless propagation is required. + */ + TX_OBSERVATION { + @Override + public String getName() { + return "spring.tx"; + } + + @Override + public String getContextualName() { + return "tx"; + } + + @Override + public TagKey[] getLowCardinalityTagKeys() { + return LowCardinalityTags.values(); + } + + @Override + public String getPrefix() { + return "spring.tx"; + } + }; + + enum LowCardinalityTags implements TagKey { + + /** + * Name of the TransactionManager. + */ + TRANSACTION_MANAGER { + @Override + public String getKey() { + return "spring.tx.transaction-manager"; + } + }, + + /** + * Whether the transaction is read-only. + */ + READ_ONLY { + @Override + public String getKey() { + return "spring.tx.read-only"; + } + }, + + /** + * Transaction propagation level. + */ + PROPAGATION_LEVEL { + @Override + public String getKey() { + return "spring.tx.propagation-level"; + } + }, + + /** + * Transaction isolation level. + */ + ISOLATION_LEVEL { + @Override + public String getKey() { + return "spring.tx.isolation-level"; + } + }, + + /** + * Transaction timeout. + */ + TIMEOUT { + @Override + public String getKey() { + return "spring.tx.timeout"; + } + }, + + /** + * Transaction name. + */ + NAME { + @Override + public String getKey() { + return "spring.tx.name"; + } + }, + + } + +} diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java new file mode 100644 index 000000000000..264ae19b9a4d --- /dev/null +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java @@ -0,0 +1,72 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.support; + +import io.micrometer.core.instrument.observation.Observation; + +import org.springframework.lang.Nullable; +import org.springframework.transaction.TransactionDefinition; +import org.springframework.transaction.TransactionManager; +import org.springframework.transaction.TransactionStatus; + +/** + * A {@link Observation.Context} for Spring Transaction. + * + * @author Marcin Grzejszczak + * @since 6.0.0 + */ +public class TransactionObservationContext extends Observation.Context { + + private final TransactionDefinition transactionDefinition; + + @Nullable + private final Class transactionManagerClass; + + private TransactionStatus status; + + private TransactionStatus transactionStatus; + + public TransactionObservationContext(@Nullable TransactionDefinition transactionDefinition, TransactionManager transactionManager) { + this.transactionDefinition = transactionDefinition != null ? transactionDefinition : TransactionDefinition.withDefaults(); + this.transactionManagerClass = transactionManager != null ? transactionManager.getClass() : null; + } + + public void setStatus(TransactionStatus status) { + this.status = status; + } + + public TransactionStatus getStatus() { + return this.status; + } + + public TransactionDefinition getTransactionDefinition() { + return this.transactionDefinition; + } + + @Nullable + public Class getTransactionManagerClass() { + return this.transactionManagerClass; + } + + public TransactionStatus getTransactionStatus() { + return this.transactionStatus; + } + + public void setTransactionStatus(TransactionStatus transactionStatus) { + this.transactionStatus = transactionStatus; + } +} diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java index 55b3b8c788c6..a94de2a28a5a 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java @@ -73,22 +73,22 @@ */ public abstract class TransactionSynchronizationManager { - private static final ThreadLocal> resources = + static final ThreadLocal> resources = new NamedThreadLocal<>("Transactional resources"); - private static final ThreadLocal> synchronizations = + static final ThreadLocal> synchronizations = new NamedThreadLocal<>("Transaction synchronizations"); - private static final ThreadLocal currentTransactionName = + static final ThreadLocal currentTransactionName = new NamedThreadLocal<>("Current transaction name"); - private static final ThreadLocal currentTransactionReadOnly = + static final ThreadLocal currentTransactionReadOnly = new NamedThreadLocal<>("Current transaction read-only status"); - private static final ThreadLocal currentTransactionIsolationLevel = + static final ThreadLocal currentTransactionIsolationLevel = new NamedThreadLocal<>("Current transaction isolation level"); - private static final ThreadLocal actualTransactionActive = + static final ThreadLocal actualTransactionActive = new NamedThreadLocal<>("Actual transaction active"); diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java new file mode 100644 index 000000000000..7c4afbf30a19 --- /dev/null +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java @@ -0,0 +1,29 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.support; + +import io.micrometer.core.instrument.observation.Observation; + +/** + * A {@link Observation.TagsProvider} for Spring Transaction. + * + * @author Marcin Grzejszczak + * @since 6.0.0 + */ +public interface TransactionTagsProvider extends Observation.TagsProvider { + +} diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java index fdbc92bb2186..1b206acc8a0f 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java @@ -18,6 +18,8 @@ import java.lang.reflect.UndeclaredThrowableException; +import io.micrometer.core.instrument.observation.Observation; +import io.micrometer.core.instrument.observation.ObservationRegistry; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -63,7 +65,7 @@ */ @SuppressWarnings("serial") public class TransactionTemplate extends DefaultTransactionDefinition - implements TransactionOperations, InitializingBean { + implements TransactionOperations, InitializingBean, Observation.TagsProviderAware { /** Logger available to subclasses. */ protected final Log logger = LogFactory.getLog(getClass()); @@ -71,6 +73,9 @@ public class TransactionTemplate extends DefaultTransactionDefinition @Nullable private PlatformTransactionManager transactionManager; + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + + private TransactionTagsProvider tagsProvider = new DefaultTransactionTagsProvider(); /** * Construct a new TransactionTemplate for bean usage. @@ -106,6 +111,7 @@ public TransactionTemplate(PlatformTransactionManager transactionManager, Transa * Set the transaction management strategy to be used. */ public void setTransactionManager(@Nullable PlatformTransactionManager transactionManager) { + // TODO: Maybe wrap this in the Observation version here? this.transactionManager = transactionManager; } @@ -117,6 +123,15 @@ public PlatformTransactionManager getTransactionManager() { return this.transactionManager; } + public void setObservationRegistry(ObservationRegistry observationRegistry) { + this.observationRegistry = observationRegistry; + } + + @Override + public void setTagsProvider(TransactionTagsProvider tagsProvider) { + this.tagsProvider = tagsProvider; + } + @Override public void afterPropertiesSet() { if (this.transactionManager == null) { @@ -131,41 +146,46 @@ public T execute(TransactionCallback action) throws TransactionException Assert.state(this.transactionManager != null, "No PlatformTransactionManager set"); if (this.transactionManager instanceof CallbackPreferringPlatformTransactionManager) { + // TODO: Should we do context propagation here for the action? return ((CallbackPreferringPlatformTransactionManager) this.transactionManager).execute(this, action); } else { - TransactionStatus status = this.transactionManager.getTransaction(this); + TransactionObservationContext context = new TransactionObservationContext(this, this.transactionManager); + ObservationPlatformTransactionManager observationPlatformTransactionManager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); + TransactionStatus status = observationPlatformTransactionManager.getTransaction(this); T result; try { + // TODO: Should we do context propagation here for the action? result = action.doInTransaction(status); } catch (RuntimeException | Error ex) { // Transactional code threw application exception -> rollback - rollbackOnException(status, ex); + rollbackOnException(observationPlatformTransactionManager, status, ex); throw ex; } catch (Throwable ex) { // Transactional code threw unexpected exception -> rollback - rollbackOnException(status, ex); + rollbackOnException(observationPlatformTransactionManager, status, ex); throw new UndeclaredThrowableException(ex, "TransactionCallback threw undeclared checked exception"); } - this.transactionManager.commit(status); + observationPlatformTransactionManager.commit(status); return result; } } /** * Perform a rollback, handling rollback exceptions properly. + * @param observationPlatformTransactionManager observation platform manager * @param status object representing the transaction * @param ex the thrown application exception or error * @throws TransactionException in case of a rollback error */ - private void rollbackOnException(TransactionStatus status, Throwable ex) throws TransactionException { + private void rollbackOnException(ObservationPlatformTransactionManager observationPlatformTransactionManager, TransactionStatus status, Throwable ex) throws TransactionException { Assert.state(this.transactionManager != null, "No PlatformTransactionManager set"); logger.debug("Initiating transaction rollback on application exception", ex); try { - this.transactionManager.rollback(status); + observationPlatformTransactionManager.rollback(status); } catch (TransactionSystemException ex2) { logger.error("Application exception overridden by rollback exception", ex); diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java new file mode 100644 index 000000000000..28f77a16e3b9 --- /dev/null +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java @@ -0,0 +1,95 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.support; + +import java.util.Map; +import java.util.Set; + +import io.micrometer.contextpropagation.ContextContainer; +import io.micrometer.contextpropagation.ThreadLocalAccessor; + +import org.springframework.lang.Nullable; + +/** + * A {@link ThreadLocalAccessor} to put and restore current transactions via {@link TransactionSynchronizationManager}. + * + * @author Marcin Grzejszczak + * @since 6.0.0 + */ +public final class TransactionThreadLocalAccessor implements ThreadLocalAccessor { + + private static final String KEY = TransactionSynchronizationManagerHolder.class.getName(); + + @Override + public void captureValues(ContextContainer container) { + TransactionSynchronizationManagerHolder value = TransactionSynchronizationManagerHolder.create(); + container.put(KEY, value); + } + + @Override + public void restoreValues(ContextContainer container) { + if (container.containsKey(KEY)) { + TransactionSynchronizationManagerHolder holder = container.get(KEY); + holder.putInScope(); + } + } + + @Override + public void resetValues(ContextContainer container) { + TransactionSynchronizationManager.clear(); + } + + static final class TransactionSynchronizationManagerHolder { + + private final Map resources; + + @Nullable + private final Set synchronizations; + + @Nullable + private final String currentTransactionName; + + private final Boolean currentTransactionReadOnly; + + @Nullable + private final Integer currentTransactionIsolationLevel; + + private final Boolean actualTransactionActive; + + private TransactionSynchronizationManagerHolder(Map resources, @Nullable Set synchronizations, @Nullable String currentTransactionName, Boolean currentTransactionReadOnly, @Nullable Integer currentTransactionIsolationLevel, Boolean actualTransactionActive) { + this.resources = resources; + this.synchronizations = synchronizations; + this.currentTransactionName = currentTransactionName; + this.currentTransactionReadOnly = currentTransactionReadOnly; + this.currentTransactionIsolationLevel = currentTransactionIsolationLevel; + this.actualTransactionActive = actualTransactionActive; + } + + private static TransactionSynchronizationManagerHolder create() { + return new TransactionSynchronizationManagerHolder(TransactionSynchronizationManager.resources.get(), TransactionSynchronizationManager.synchronizations.get(), TransactionSynchronizationManager.currentTransactionName.get(), TransactionSynchronizationManager.currentTransactionReadOnly.get(), TransactionSynchronizationManager.currentTransactionIsolationLevel.get(), TransactionSynchronizationManager.actualTransactionActive.get()); + } + + private void putInScope() { + TransactionSynchronizationManager.resources.set(this.resources); + TransactionSynchronizationManager.synchronizations.set(this.synchronizations); + TransactionSynchronizationManager.currentTransactionName.set(this.currentTransactionName); + TransactionSynchronizationManager.currentTransactionReadOnly.set(this.currentTransactionReadOnly); + TransactionSynchronizationManager.currentTransactionIsolationLevel.set(this.currentTransactionIsolationLevel); + TransactionSynchronizationManager.actualTransactionActive.set(this.actualTransactionActive); + } + } +} diff --git a/spring-tx/src/main/resources/META-INF/services/io.micrometer.contextpropagation.ThreadLocalAccessor b/spring-tx/src/main/resources/META-INF/services/io.micrometer.contextpropagation.ThreadLocalAccessor new file mode 100644 index 000000000000..0913a378225d --- /dev/null +++ b/spring-tx/src/main/resources/META-INF/services/io.micrometer.contextpropagation.ThreadLocalAccessor @@ -0,0 +1 @@ +org.springframework.transaction.support.TransactionThreadLocalAccessor From d2b71227f147d75beb94868855d9beee5f58a660 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 11 Mar 2022 13:03:25 +0100 Subject: [PATCH 02/15] Changes following the review --- .../scheduling/quartz/SchedulerAccessor.java | 5 +- .../DefaultTransactionTagsProvider.java | 51 +++++++------------ ...ObservationPlatformTransactionManager.java | 35 +++++-------- .../support/TransactionObservation.java | 15 ++++-- .../TransactionObservationContext.java | 35 ++++++++----- .../support/TransactionTemplate.java | 2 - .../TransactionThreadLocalAccessor.java | 21 ++++++++ .../TransactionalEventListenerTests.java | 4 ++ 8 files changed, 94 insertions(+), 74 deletions(-) diff --git a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java index 57329988f5d3..d7b2b2adc9b7 100644 --- a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java +++ b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java @@ -224,9 +224,10 @@ public void setTagsProvider(TransactionTagsProvider tagsProvider) { protected void registerJobsAndTriggers() throws SchedulerException { TransactionStatus transactionStatus = null; TransactionDefinition definition = TransactionDefinition.withDefaults(); - TransactionObservationContext context = new TransactionObservationContext(definition, this.transactionManager); - ObservationPlatformTransactionManager observationPlatformTransactionManager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); + ObservationPlatformTransactionManager observationPlatformTransactionManager = null; if (this.transactionManager != null) { + TransactionObservationContext context = new TransactionObservationContext(definition, this.transactionManager); + observationPlatformTransactionManager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); transactionStatus = observationPlatformTransactionManager.getTransaction(definition); } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java index c33e622ac317..b3c3f523a840 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java @@ -20,6 +20,8 @@ import io.micrometer.core.instrument.observation.Observation; import org.springframework.transaction.TransactionDefinition; +import org.springframework.transaction.annotation.Isolation; +import org.springframework.transaction.annotation.Propagation; import org.springframework.util.ClassUtils; import org.springframework.util.StringUtils; @@ -31,6 +33,10 @@ */ public class DefaultTransactionTagsProvider implements TransactionTagsProvider { + private static final Propagation[] PROPAGATIONS = Propagation.values(); + + private static final Isolation[] ISOLATIONS = Isolation.values(); + @Override public Tags getLowCardinalityTags(TransactionObservationContext context) { if (!context.getTransactionStatus().isNewTransaction()) { @@ -41,56 +47,37 @@ public Tags getLowCardinalityTags(TransactionObservationContext context) { if (transactionDefinition.getTimeout() > 0) { tags = tags.and(TransactionObservation.LowCardinalityTags.TIMEOUT.of(String.valueOf(transactionDefinition.getTimeout()))); } - if (StringUtils.hasText(transactionDefinition.getName())) { - tags = tags.and(TransactionObservation.LowCardinalityTags.NAME.of(transactionDefinition.getName())); - } return tags.and(TransactionObservation.LowCardinalityTags.TRANSACTION_MANAGER.of(ClassUtils.getQualifiedName(context.getTransactionManagerClass())), TransactionObservation.LowCardinalityTags.READ_ONLY.of(String.valueOf(transactionDefinition.isReadOnly())), TransactionObservation.LowCardinalityTags.PROPAGATION_LEVEL.of(propagationLevel(transactionDefinition)), TransactionObservation.LowCardinalityTags.ISOLATION_LEVEL.of(isolationLevel(transactionDefinition))); } + @Override + public Tags getHighCardinalityTags(TransactionObservationContext context) { + TransactionDefinition transactionDefinition = context.getTransactionDefinition(); + if (StringUtils.hasText(transactionDefinition.getName())) { + return Tags.of(TransactionObservation.HighCardinalityTags.NAME.of(transactionDefinition.getName())); + } + return Tags.empty(); + } + @Override public boolean supportsContext(Observation.Context context) { return context instanceof TransactionObservationContext; } - private static String propagationLevel(TransactionDefinition def) { - switch (def.getPropagationBehavior()) { - case 0: - return "PROPAGATION_REQUIRED"; - case 1: - return "PROPAGATION_SUPPORTS"; - case 2: - return "PROPAGATION_MANDATORY"; - case 3: - return "PROPAGATION_REQUIRES_NEW"; - case 4: - return "PROPAGATION_NOT_SUPPORTED"; - case 5: - return "PROPAGATION_NEVER"; - case 6: - return "PROPAGATION_NESTED"; - default: + if (def.getPropagationBehavior() < 0 || def.getPropagationBehavior() >= PROPAGATIONS.length) { return String.valueOf(def.getPropagationBehavior()); } + return PROPAGATIONS[def.getPropagationBehavior()].name(); } private static String isolationLevel(TransactionDefinition def) { - switch (def.getIsolationLevel()) { - case -1: - return "ISOLATION_DEFAULT"; - case 1: - return "ISOLATION_READ_UNCOMMITTED"; - case 2: - return "ISOLATION_READ_COMMITTED"; - case 4: - return "ISOLATION_REPEATABLE_READ"; - case 8: - return "ISOLATION_SERIALIZABLE"; - default: + if (def.getIsolationLevel() < 0 || def.getIsolationLevel() >= ISOLATIONS.length) { return String.valueOf(def.getIsolationLevel()); } + return ISOLATIONS[def.getIsolationLevel()].name(); } } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java index 38bdd4e0519e..78ce95c2002a 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java @@ -44,10 +44,6 @@ public final class ObservationPlatformTransactionManager implements PlatformTran private final TransactionTagsProvider transactionTagsProvider; - private Observation observation; - - private Observation.Scope scope; - public ObservationPlatformTransactionManager(PlatformTransactionManager delegate, ObservationRegistry observationRegistry, TransactionObservationContext context, TransactionTagsProvider transactionTagsProvider) { this.delegate = delegate; this.observationRegistry = observationRegistry; @@ -57,31 +53,31 @@ public ObservationPlatformTransactionManager(PlatformTransactionManager delegate @Override public TransactionStatus getTransaction(TransactionDefinition definition) throws TransactionException { + if (this.observationRegistry.isNoOp()) { + return this.delegate.getTransaction(definition); + } Observation obs = this.observationRegistry.getCurrentObservation(); if (obs == null) { obs = TransactionObservation.TX_OBSERVATION.observation(this.observationRegistry, this.context) .tagsProvider(this.transactionTagsProvider) .start(); + this.context.setScope(obs.openScope()); } - this.observation = obs; - this.scope = this.observation.openScope(); + this.context.setObservation(obs); try { TransactionStatus transaction = this.delegate.getTransaction(definition); this.context.setTransactionStatus(transaction); return transaction; } catch (Exception ex) { - this.observation.error(ex).stop(); + this.context.getObservation().error(ex).stop(); throw ex; } } @Override public void commit(TransactionStatus status) throws TransactionException { - if (this.observation == null) { - if (log.isDebugEnabled()) { - log.debug("Commit called without calling getTransaction first - this shouldn't happen, sth is wrong"); - } + if (this.observationRegistry.isNoOp()) { this.delegate.commit(status); return; } @@ -92,21 +88,18 @@ public void commit(TransactionStatus status) throws TransactionException { this.delegate.commit(status); } catch (Exception ex) { - this.observation.error(ex); + this.context.getObservation().error(ex); throw ex; } finally { - this.scope.close(); - this.observation.stop(); + this.context.getScope().close(); + this.context.getObservation().stop(); } } @Override public void rollback(TransactionStatus status) throws TransactionException { - if (this.observation == null) { - if (log.isDebugEnabled()) { - log.debug("No observation found - this shouldn't happen, sth is wrong"); - } + if (this.observationRegistry.isNoOp()) { this.delegate.rollback(status); return; } @@ -117,12 +110,12 @@ public void rollback(TransactionStatus status) throws TransactionException { this.delegate.rollback(status); } catch (Exception ex) { - this.observation.error(ex); + this.context.getObservation().error(ex); throw ex; } finally { - this.scope.close(); - this.observation.stop(); + this.context.getScope().close(); + this.context.getObservation().stop(); } } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java index 6e3ae4a23da0..84179eb0c52a 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java @@ -41,6 +41,11 @@ public TagKey[] getLowCardinalityTagKeys() { return LowCardinalityTags.values(); } + @Override + public TagKey[] getHighCardinalityTagKeys() { + return HighCardinalityTags.values(); + } + @Override public String getPrefix() { return "spring.tx"; @@ -97,7 +102,11 @@ public String getKey() { public String getKey() { return "spring.tx.timeout"; } - }, + } + + } + + enum HighCardinalityTags implements TagKey { /** * Transaction name. @@ -107,8 +116,6 @@ public String getKey() { public String getKey() { return "spring.tx.name"; } - }, - + } } - } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java index 264ae19b9a4d..a72238160e11 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java @@ -16,6 +16,7 @@ package org.springframework.transaction.support; +import io.micrometer.core.instrument.observation.NoopObservation; import io.micrometer.core.instrument.observation.Observation; import org.springframework.lang.Nullable; @@ -33,31 +34,23 @@ public class TransactionObservationContext extends Observation.Context { private final TransactionDefinition transactionDefinition; - @Nullable private final Class transactionManagerClass; - private TransactionStatus status; - private TransactionStatus transactionStatus; - public TransactionObservationContext(@Nullable TransactionDefinition transactionDefinition, TransactionManager transactionManager) { - this.transactionDefinition = transactionDefinition != null ? transactionDefinition : TransactionDefinition.withDefaults(); - this.transactionManagerClass = transactionManager != null ? transactionManager.getClass() : null; - } + private Observation observation = NoopObservation.INSTANCE; - public void setStatus(TransactionStatus status) { - this.status = status; - } + private Observation.Scope scope = NoopObservation.NoOpScope.INSTANCE; - public TransactionStatus getStatus() { - return this.status; + public TransactionObservationContext(@Nullable TransactionDefinition transactionDefinition, TransactionManager transactionManager) { + this.transactionDefinition = transactionDefinition != null ? transactionDefinition : TransactionDefinition.withDefaults(); + this.transactionManagerClass = transactionManager.getClass(); } public TransactionDefinition getTransactionDefinition() { return this.transactionDefinition; } - @Nullable public Class getTransactionManagerClass() { return this.transactionManagerClass; } @@ -69,4 +62,20 @@ public TransactionStatus getTransactionStatus() { public void setTransactionStatus(TransactionStatus transactionStatus) { this.transactionStatus = transactionStatus; } + + public Observation getObservation() { + return this.observation; + } + + public void setObservation(Observation observation) { + this.observation = observation; + } + + public Observation.Scope getScope() { + return this.scope; + } + + public void setScope(Observation.Scope scope) { + this.scope = scope; + } } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java index 1b206acc8a0f..e140c5ceda47 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java @@ -146,7 +146,6 @@ public T execute(TransactionCallback action) throws TransactionException Assert.state(this.transactionManager != null, "No PlatformTransactionManager set"); if (this.transactionManager instanceof CallbackPreferringPlatformTransactionManager) { - // TODO: Should we do context propagation here for the action? return ((CallbackPreferringPlatformTransactionManager) this.transactionManager).execute(this, action); } else { @@ -155,7 +154,6 @@ public T execute(TransactionCallback action) throws TransactionException TransactionStatus status = observationPlatformTransactionManager.getTransaction(this); T result; try { - // TODO: Should we do context propagation here for the action? result = action.doInTransaction(status); } catch (RuntimeException | Error ex) { diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java index 28f77a16e3b9..f38ad8d8b648 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java @@ -21,8 +21,11 @@ import io.micrometer.contextpropagation.ContextContainer; import io.micrometer.contextpropagation.ThreadLocalAccessor; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.lang.Nullable; +import org.springframework.util.StringUtils; /** * A {@link ThreadLocalAccessor} to put and restore current transactions via {@link TransactionSynchronizationManager}. @@ -32,8 +35,12 @@ */ public final class TransactionThreadLocalAccessor implements ThreadLocalAccessor { + private static final Log log = LogFactory.getLog(TransactionThreadLocalAccessor.class); + private static final String KEY = TransactionSynchronizationManagerHolder.class.getName(); + private static final String PREVIOUS_TRANSACTION_KEY = TransactionSynchronizationManagerHolder.class.getName() + "_PREVIOUS_TRANSACTION_PRESENT"; + @Override public void captureValues(ContextContainer container) { TransactionSynchronizationManagerHolder value = TransactionSynchronizationManagerHolder.create(); @@ -42,6 +49,14 @@ public void captureValues(ContextContainer container) { @Override public void restoreValues(ContextContainer container) { + String transactionName = TransactionSynchronizationManager.getCurrentTransactionName(); + if (StringUtils.hasText(transactionName)) { + container.put(PREVIOUS_TRANSACTION_KEY, true); + if (log.isDebugEnabled()) { + log.debug("A transaction with name [" + transactionName + "] has already been set in this thread. We don't want to override it. Will not update the synchronization manager with entries from the container"); + } + return; + } if (container.containsKey(KEY)) { TransactionSynchronizationManagerHolder holder = container.get(KEY); holder.putInScope(); @@ -50,6 +65,12 @@ public void restoreValues(ContextContainer container) { @Override public void resetValues(ContextContainer container) { + if (Boolean.parseBoolean(String.valueOf(container.get(PREVIOUS_TRANSACTION_KEY)))) { + if (log.isDebugEnabled()) { + log.debug("There was a transaction when context propagation happened. Will not reset the synchronization manager."); + } + return; + } TransactionSynchronizationManager.clear(); } diff --git a/spring-tx/src/test/java/org/springframework/transaction/event/TransactionalEventListenerTests.java b/spring-tx/src/test/java/org/springframework/transaction/event/TransactionalEventListenerTests.java index 22ed99a2b9aa..14842aa1e665 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/event/TransactionalEventListenerTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/event/TransactionalEventListenerTests.java @@ -138,7 +138,11 @@ public void afterCompletionRollback() { @Test public void afterCommit() { load(AfterCompletionExplicitTestListener.class); + // Nie robic context propagation + // jezeli tu byl span this.transactionTemplate.execute(status -> { + // jezeli jest ixNewTransaction + // TSM -> getTransaction (zalozmy, ze new) TX 2 ? getContext().publishEvent("test"); getEventCollector().assertNoEventReceived(); return null; From 9bb6a58d5e62a2f35a2c2cdaffb3f91aca2b6fb9 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 11 Mar 2022 15:43:24 +0100 Subject: [PATCH 03/15] Added a test for a transactiontemplate --- spring-tx/spring-tx.gradle | 1 + ...ObservationPlatformTransactionManager.java | 6 +- .../support/TransactionTemplate.java | 1 - .../ObservationTransactionTemplateTests.java | 79 +++++++++++++++++++ 4 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java diff --git a/spring-tx/spring-tx.gradle b/spring-tx/spring-tx.gradle index 26e1229333a8..a11124651f94 100644 --- a/spring-tx/spring-tx.gradle +++ b/spring-tx/spring-tx.gradle @@ -27,4 +27,5 @@ dependencies { testImplementation("org.apache.groovy:groovy") testImplementation("jakarta.persistence:jakarta.persistence-api") testImplementation("io.projectreactor:reactor-test") + testImplementation("io.micrometer:micrometer-tracing-integration-test") } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java index 78ce95c2002a..24c842e2f1ac 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java @@ -16,6 +16,7 @@ package org.springframework.transaction.support; +import io.micrometer.core.instrument.observation.NoopObservation; import io.micrometer.core.instrument.observation.Observation; import io.micrometer.core.instrument.observation.ObservationRegistry; import org.apache.commons.logging.Log; @@ -57,11 +58,12 @@ public TransactionStatus getTransaction(TransactionDefinition definition) throws return this.delegate.getTransaction(definition); } Observation obs = this.observationRegistry.getCurrentObservation(); - if (obs == null) { + obs = obs != null ? obs : NoopObservation.INSTANCE; + try (Observation.Scope scope = obs.openScope()) { obs = TransactionObservation.TX_OBSERVATION.observation(this.observationRegistry, this.context) .tagsProvider(this.transactionTagsProvider) .start(); - this.context.setScope(obs.openScope()); + this.context.setScope(scope); } this.context.setObservation(obs); try { diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java index e140c5ceda47..548b2060bf24 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java @@ -111,7 +111,6 @@ public TransactionTemplate(PlatformTransactionManager transactionManager, Transa * Set the transaction management strategy to be used. */ public void setTransactionManager(@Nullable PlatformTransactionManager transactionManager) { - // TODO: Maybe wrap this in the Observation version here? this.transactionManager = transactionManager; } diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java new file mode 100644 index 000000000000..b7c4f374aaa2 --- /dev/null +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java @@ -0,0 +1,79 @@ +/* + * Copyright 2002-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.interceptor; + +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import io.micrometer.core.tck.MeterRegistryAssert; +import io.micrometer.tracing.test.SampleTestRunner; +import io.micrometer.tracing.test.reporter.BuildingBlocks; +import io.micrometer.tracing.test.simple.SpansAssert; + +import org.springframework.transaction.support.TransactionCallback; +import org.springframework.transaction.support.TransactionTemplate; +import org.springframework.transaction.testfixture.CallCountingTransactionManager; + +import static org.assertj.core.api.BDDAssertions.then; + +/** + * + * @author Marcin Grzejszczak + * @since 6.0.0 + */ +class ObservationTransactionTemplateTests extends SampleTestRunner { + + ObservationTransactionTemplateTests() { + super(SampleRunnerConfig.builder().build(), new SimpleMeterRegistry().withTimerObservationHandler()); + } + + @Override + public SampleTestRunnerConsumer yourCode() throws Exception { + return (bb, meterRegistry) -> { + TransactionTemplate transactionTemplate = new TransactionTemplate(); + transactionTemplate.setTransactionManager(new CallCountingTransactionManager()); + transactionTemplate.setObservationRegistry(meterRegistry); + + then(transactionTemplate.execute((TransactionCallback) status -> "hello")).isEqualTo("hello"); + + Tag isolationLevelTag = Tag.of("spring.tx.isolation-level", "-1"); + Tag propagationLevelTag = Tag.of("spring.tx.propagation-level", "REQUIRED"); + Tag readOnlyTag = Tag.of("spring.tx.read-only", "false"); + Tag transactionManagerTag = Tag.of("spring.tx.transaction-manager", "org.springframework.transaction.testfixture.CallCountingTransactionManager"); + thenMeterRegistryHasATxMetric(meterRegistry, isolationLevelTag, propagationLevelTag, readOnlyTag, transactionManagerTag); + thenThereIsATxSpan(bb, isolationLevelTag, propagationLevelTag, readOnlyTag, transactionManagerTag); + }; + } + + private void thenMeterRegistryHasATxMetric(MeterRegistry meterRegistry, Tag isolationLevelTag, Tag propagationLevelTag, Tag readOnlyTag, Tag transactionManagerTag) { + MeterRegistryAssert.then(meterRegistry) + .hasTimerWithNameAndTags("spring.tx", + Tags.of(isolationLevelTag, propagationLevelTag, readOnlyTag, transactionManagerTag)); + } + + private void thenThereIsATxSpan(BuildingBlocks bb, Tag isolationLevelTag, Tag propagationLevelTag, Tag readOnlyTag, Tag transactionManagerTag) { + SpansAssert.then(bb.getFinishedSpans()) + .thenASpanWithNameEqualTo("tx") + .hasTag(isolationLevelTag.getKey(), isolationLevelTag.getValue()) + .hasTag(propagationLevelTag.getKey(), propagationLevelTag.getValue()) + .hasTag(readOnlyTag.getKey(), readOnlyTag.getValue()) + .hasTag(transactionManagerTag.getKey(), transactionManagerTag.getValue()) + .backToSpans() + .hasSize(1); + } +} From 0a7ab4fcda4dd623141c1d5c25f7d3b410f91b40 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 11 Mar 2022 15:48:54 +0100 Subject: [PATCH 04/15] Removed polish comments ;) --- .../transaction/event/TransactionalEventListenerTests.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/spring-tx/src/test/java/org/springframework/transaction/event/TransactionalEventListenerTests.java b/spring-tx/src/test/java/org/springframework/transaction/event/TransactionalEventListenerTests.java index 14842aa1e665..22ed99a2b9aa 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/event/TransactionalEventListenerTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/event/TransactionalEventListenerTests.java @@ -138,11 +138,7 @@ public void afterCompletionRollback() { @Test public void afterCommit() { load(AfterCompletionExplicitTestListener.class); - // Nie robic context propagation - // jezeli tu byl span this.transactionTemplate.execute(status -> { - // jezeli jest ixNewTransaction - // TSM -> getTransaction (zalozmy, ze new) TX 2 ? getContext().publishEvent("test"); getEventCollector().assertNoEventReceived(); return null; From 46a04285c68aabae75b7443e77925ba3ab691008 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 11 Mar 2022 15:59:20 +0100 Subject: [PATCH 05/15] Updated code to the latest micrometer changes --- .../support/ObservationPlatformTransactionManager.java | 3 +-- .../transaction/support/TransactionObservationContext.java | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java index 24c842e2f1ac..911866e952c5 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java @@ -16,7 +16,6 @@ package org.springframework.transaction.support; -import io.micrometer.core.instrument.observation.NoopObservation; import io.micrometer.core.instrument.observation.Observation; import io.micrometer.core.instrument.observation.ObservationRegistry; import org.apache.commons.logging.Log; @@ -58,7 +57,7 @@ public TransactionStatus getTransaction(TransactionDefinition definition) throws return this.delegate.getTransaction(definition); } Observation obs = this.observationRegistry.getCurrentObservation(); - obs = obs != null ? obs : NoopObservation.INSTANCE; + obs = obs != null ? obs : Observation.NOOP; try (Observation.Scope scope = obs.openScope()) { obs = TransactionObservation.TX_OBSERVATION.observation(this.observationRegistry, this.context) .tagsProvider(this.transactionTagsProvider) diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java index a72238160e11..8df652890dbc 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java @@ -16,7 +16,6 @@ package org.springframework.transaction.support; -import io.micrometer.core.instrument.observation.NoopObservation; import io.micrometer.core.instrument.observation.Observation; import org.springframework.lang.Nullable; @@ -38,9 +37,9 @@ public class TransactionObservationContext extends Observation.Context { private TransactionStatus transactionStatus; - private Observation observation = NoopObservation.INSTANCE; + private Observation observation = Observation.NOOP; - private Observation.Scope scope = NoopObservation.NoOpScope.INSTANCE; + private Observation.Scope scope = Observation.Scope.NOOP; public TransactionObservationContext(@Nullable TransactionDefinition transactionDefinition, TransactionManager transactionManager) { this.transactionDefinition = transactionDefinition != null ? transactionDefinition : TransactionDefinition.withDefaults(); From e0ca6e2534afa31a23f02f57d837d80746030777 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Tue, 15 Mar 2022 17:18:04 +0100 Subject: [PATCH 06/15] Added more tests --- .../spring-context-support.gradle | 1 + .../ObservationSchedulerAccessorTests.java | 70 +++++++++++++++++ spring-tx/spring-tx.gradle | 2 +- .../ObservationTransactionTemplateTests.java | 53 +++---------- ...ionTransactionManagerSampleTestRunner.java | 76 +++++++++++++++++++ 5 files changed, 159 insertions(+), 43 deletions(-) create mode 100644 spring-context-support/src/test/java/org/springframework/scheduling/quartz/ObservationSchedulerAccessorTests.java create mode 100644 spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java diff --git a/spring-context-support/spring-context-support.gradle b/spring-context-support/spring-context-support.gradle index 160f8028985c..c0a67b3f79fe 100644 --- a/spring-context-support/spring-context-support.gradle +++ b/spring-context-support/spring-context-support.gradle @@ -26,4 +26,5 @@ dependencies { testFixturesApi("org.junit.jupiter:junit-jupiter-api") testFixturesImplementation("org.assertj:assertj-core") testFixturesImplementation("org.mockito:mockito-core") + testImplementation("io.micrometer:micrometer-tracing-integration-test") } diff --git a/spring-context-support/src/test/java/org/springframework/scheduling/quartz/ObservationSchedulerAccessorTests.java b/spring-context-support/src/test/java/org/springframework/scheduling/quartz/ObservationSchedulerAccessorTests.java new file mode 100644 index 000000000000..ab388be0ca4e --- /dev/null +++ b/spring-context-support/src/test/java/org/springframework/scheduling/quartz/ObservationSchedulerAccessorTests.java @@ -0,0 +1,70 @@ +/* + * Copyright 2002-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.scheduling.quartz; + +import java.util.HashMap; +import java.util.Map; + +import io.micrometer.core.instrument.observation.ObservationRegistry; +import org.mockito.BDDMockito; +import org.quartz.Scheduler; +import org.quartz.SchedulerContext; +import org.quartz.SchedulerFactory; + +import org.springframework.beans.testfixture.beans.TestBean; +import org.springframework.context.support.StaticApplicationContext; +import org.springframework.transaction.testfixture.CallCountingTransactionManager; +import org.springframework.transaction.testfixture.ObservationTransactionManagerSampleTestRunner; + +import static org.mockito.Mockito.mock; + +/** + * @author Marcin Grzejszczak + */ +class ObservationSchedulerAccessorTests extends ObservationTransactionManagerSampleTestRunner { + + @Override + protected SchedulerFactoryBean given(ObservationRegistry observationRegistry) throws Exception { + // given + TestBean tb = new TestBean("tb", 99); + StaticApplicationContext ac = new StaticApplicationContext(); + final Scheduler scheduler = mock(Scheduler.class); + SchedulerContext schedulerContext = new SchedulerContext(); + BDDMockito.given(scheduler.getContext()).willReturn(schedulerContext); + SchedulerFactoryBean schedulerFactoryBean = new SchedulerFactoryBean() { + @Override + protected Scheduler createScheduler(SchedulerFactory schedulerFactory, String schedulerName) { + return scheduler; + } + }; + schedulerFactoryBean.setJobFactory(null); + Map schedulerContextMap = new HashMap<>(); + schedulerContextMap.put("testBean", tb); + schedulerFactoryBean.setSchedulerContextAsMap(schedulerContextMap); + schedulerFactoryBean.setApplicationContext(ac); + schedulerFactoryBean.setApplicationContextSchedulerContextKey("appCtx"); + schedulerFactoryBean.setTransactionManager(new CallCountingTransactionManager()); + schedulerFactoryBean.setObservationRegistry(observationRegistry); + return schedulerFactoryBean; + } + + @Override + protected void when(SchedulerFactoryBean sut) throws Exception { + sut.afterPropertiesSet(); + sut.start(); + } +} diff --git a/spring-tx/spring-tx.gradle b/spring-tx/spring-tx.gradle index a11124651f94..2d0f9958028e 100644 --- a/spring-tx/spring-tx.gradle +++ b/spring-tx/spring-tx.gradle @@ -27,5 +27,5 @@ dependencies { testImplementation("org.apache.groovy:groovy") testImplementation("jakarta.persistence:jakarta.persistence-api") testImplementation("io.projectreactor:reactor-test") - testImplementation("io.micrometer:micrometer-tracing-integration-test") + optional("io.micrometer:micrometer-tracing-integration-test") // I don't know how to make this be in tests and testFixtures } diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java index b7c4f374aaa2..9c85686621e0 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java @@ -16,18 +16,12 @@ package org.springframework.transaction.interceptor; -import io.micrometer.core.instrument.MeterRegistry; -import io.micrometer.core.instrument.Tag; -import io.micrometer.core.instrument.Tags; -import io.micrometer.core.instrument.simple.SimpleMeterRegistry; -import io.micrometer.core.tck.MeterRegistryAssert; -import io.micrometer.tracing.test.SampleTestRunner; -import io.micrometer.tracing.test.reporter.BuildingBlocks; -import io.micrometer.tracing.test.simple.SpansAssert; +import io.micrometer.core.instrument.observation.ObservationRegistry; import org.springframework.transaction.support.TransactionCallback; import org.springframework.transaction.support.TransactionTemplate; import org.springframework.transaction.testfixture.CallCountingTransactionManager; +import org.springframework.transaction.testfixture.ObservationTransactionManagerSampleTestRunner; import static org.assertj.core.api.BDDAssertions.then; @@ -36,44 +30,19 @@ * @author Marcin Grzejszczak * @since 6.0.0 */ -class ObservationTransactionTemplateTests extends SampleTestRunner { - - ObservationTransactionTemplateTests() { - super(SampleRunnerConfig.builder().build(), new SimpleMeterRegistry().withTimerObservationHandler()); - } +class ObservationTransactionTemplateTests extends ObservationTransactionManagerSampleTestRunner { @Override - public SampleTestRunnerConsumer yourCode() throws Exception { - return (bb, meterRegistry) -> { - TransactionTemplate transactionTemplate = new TransactionTemplate(); - transactionTemplate.setTransactionManager(new CallCountingTransactionManager()); - transactionTemplate.setObservationRegistry(meterRegistry); - - then(transactionTemplate.execute((TransactionCallback) status -> "hello")).isEqualTo("hello"); - - Tag isolationLevelTag = Tag.of("spring.tx.isolation-level", "-1"); - Tag propagationLevelTag = Tag.of("spring.tx.propagation-level", "REQUIRED"); - Tag readOnlyTag = Tag.of("spring.tx.read-only", "false"); - Tag transactionManagerTag = Tag.of("spring.tx.transaction-manager", "org.springframework.transaction.testfixture.CallCountingTransactionManager"); - thenMeterRegistryHasATxMetric(meterRegistry, isolationLevelTag, propagationLevelTag, readOnlyTag, transactionManagerTag); - thenThereIsATxSpan(bb, isolationLevelTag, propagationLevelTag, readOnlyTag, transactionManagerTag); - }; + protected TransactionTemplate given(ObservationRegistry observationRegistry) { + TransactionTemplate transactionTemplate = new TransactionTemplate(); + transactionTemplate.setTransactionManager(new CallCountingTransactionManager()); + transactionTemplate.setObservationRegistry(observationRegistry); + return transactionTemplate; } - private void thenMeterRegistryHasATxMetric(MeterRegistry meterRegistry, Tag isolationLevelTag, Tag propagationLevelTag, Tag readOnlyTag, Tag transactionManagerTag) { - MeterRegistryAssert.then(meterRegistry) - .hasTimerWithNameAndTags("spring.tx", - Tags.of(isolationLevelTag, propagationLevelTag, readOnlyTag, transactionManagerTag)); + @Override + protected void when(TransactionTemplate sut) { + then(sut.execute((TransactionCallback) status -> "hello")).isEqualTo("hello"); } - private void thenThereIsATxSpan(BuildingBlocks bb, Tag isolationLevelTag, Tag propagationLevelTag, Tag readOnlyTag, Tag transactionManagerTag) { - SpansAssert.then(bb.getFinishedSpans()) - .thenASpanWithNameEqualTo("tx") - .hasTag(isolationLevelTag.getKey(), isolationLevelTag.getValue()) - .hasTag(propagationLevelTag.getKey(), propagationLevelTag.getValue()) - .hasTag(readOnlyTag.getKey(), readOnlyTag.getValue()) - .hasTag(transactionManagerTag.getKey(), transactionManagerTag.getValue()) - .backToSpans() - .hasSize(1); - } } diff --git a/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java b/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java new file mode 100644 index 000000000000..10721cb8c263 --- /dev/null +++ b/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java @@ -0,0 +1,76 @@ +/* + * Copyright 2002-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.testfixture; + +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; +import io.micrometer.core.instrument.observation.ObservationRegistry; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import io.micrometer.core.tck.MeterRegistryAssert; +import io.micrometer.tracing.test.SampleTestRunner; +import io.micrometer.tracing.test.reporter.BuildingBlocks; +import io.micrometer.tracing.test.simple.SpansAssert; + +/** + * + * @author Marcin Grzejszczak + * @since 6.0.0 + */ +public abstract class ObservationTransactionManagerSampleTestRunner extends SampleTestRunner { + + public ObservationTransactionManagerSampleTestRunner() { + super(SampleRunnerConfig.builder().build(), new SimpleMeterRegistry().withTimerObservationHandler()); + } + + @Override + public SampleTestRunnerConsumer yourCode() throws Exception { + return (bb, meterRegistry) -> { + T sut = given(meterRegistry); + + when(sut); + + Tag isolationLevelTag = Tag.of("spring.tx.isolation-level", "-1"); + Tag propagationLevelTag = Tag.of("spring.tx.propagation-level", "REQUIRED"); + Tag readOnlyTag = Tag.of("spring.tx.read-only", "false"); + Tag transactionManagerTag = Tag.of("spring.tx.transaction-manager", "org.springframework.transaction.testfixture.CallCountingTransactionManager"); + thenMeterRegistryHasATxMetric(meterRegistry, isolationLevelTag, propagationLevelTag, readOnlyTag, transactionManagerTag); + thenThereIsATxSpan(bb, isolationLevelTag, propagationLevelTag, readOnlyTag, transactionManagerTag); + }; + } + + protected abstract T given(ObservationRegistry observationRegistry) throws Exception; + + protected abstract void when(T sut) throws Exception; + + private void thenMeterRegistryHasATxMetric(MeterRegistry meterRegistry, Tag isolationLevelTag, Tag propagationLevelTag, Tag readOnlyTag, Tag transactionManagerTag) { + MeterRegistryAssert.then(meterRegistry) + .hasTimerWithNameAndTags("spring.tx", + Tags.of(isolationLevelTag, propagationLevelTag, readOnlyTag, transactionManagerTag)); + } + + private void thenThereIsATxSpan(BuildingBlocks bb, Tag isolationLevelTag, Tag propagationLevelTag, Tag readOnlyTag, Tag transactionManagerTag) { + SpansAssert.then(bb.getFinishedSpans()) + .thenASpanWithNameEqualTo("tx") + .hasTag(isolationLevelTag.getKey(), isolationLevelTag.getValue()) + .hasTag(propagationLevelTag.getKey(), propagationLevelTag.getValue()) + .hasTag(readOnlyTag.getKey(), readOnlyTag.getValue()) + .hasTag(transactionManagerTag.getKey(), transactionManagerTag.getValue()) + .backToSpans() + .hasSize(1); + } +} From 4e6a01934163c73549b09fbf3bca6c4c6266f3b1 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 21 Mar 2022 10:56:57 +0100 Subject: [PATCH 07/15] Better tests and updated the API to the latest micrometer changes --- .../spring-context-support.gradle | 1 + .../scheduling/quartz/SchedulerAccessor.java | 4 +- .../ObservationSchedulerAccessorTests.java | 7 +- spring-jms/spring-jms.gradle | 1 + ...stractPollingMessageListenerContainer.java | 4 +- spring-tx/spring-tx.gradle | 4 +- .../interceptor/TransactionAspectSupport.java | 4 +- .../DefaultTransactionTagsProvider.java | 4 +- ...ObservationPlatformTransactionManager.java | 4 +- .../support/TransactionObservation.java | 4 +- .../TransactionObservationContext.java | 2 +- .../support/TransactionTagsProvider.java | 2 +- .../support/TransactionTemplate.java | 4 +- .../ObservationTransactionTemplateTests.java | 2 +- ...vationPlatformTransactionManagerTests.java | 115 ++++++++++++++++++ ...ionTransactionManagerSampleTestRunner.java | 7 +- 16 files changed, 145 insertions(+), 24 deletions(-) create mode 100644 spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java diff --git a/spring-context-support/spring-context-support.gradle b/spring-context-support/spring-context-support.gradle index c0a67b3f79fe..bb73e60e50dc 100644 --- a/spring-context-support/spring-context-support.gradle +++ b/spring-context-support/spring-context-support.gradle @@ -4,6 +4,7 @@ dependencies { api(project(":spring-beans")) api(project(":spring-context")) api(project(":spring-core")) + api("io.micrometer:micrometer-observation") optional(project(":spring-jdbc")) // for Quartz support optional(project(":spring-tx")) // for Quartz support optional("jakarta.activation:jakarta.activation-api") diff --git a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java index d7b2b2adc9b7..a272e8777d81 100644 --- a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java +++ b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java @@ -21,8 +21,8 @@ import java.util.List; import java.util.Map; -import io.micrometer.core.instrument.observation.Observation; -import io.micrometer.core.instrument.observation.ObservationRegistry; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.quartz.Calendar; diff --git a/spring-context-support/src/test/java/org/springframework/scheduling/quartz/ObservationSchedulerAccessorTests.java b/spring-context-support/src/test/java/org/springframework/scheduling/quartz/ObservationSchedulerAccessorTests.java index ab388be0ca4e..9c24b21dd043 100644 --- a/spring-context-support/src/test/java/org/springframework/scheduling/quartz/ObservationSchedulerAccessorTests.java +++ b/spring-context-support/src/test/java/org/springframework/scheduling/quartz/ObservationSchedulerAccessorTests.java @@ -19,7 +19,7 @@ import java.util.HashMap; import java.util.Map; -import io.micrometer.core.instrument.observation.ObservationRegistry; +import io.micrometer.observation.ObservationRegistry; import org.mockito.BDDMockito; import org.quartz.Scheduler; import org.quartz.SchedulerContext; @@ -39,12 +39,15 @@ class ObservationSchedulerAccessorTests extends ObservationTransactionManagerSam @Override protected SchedulerFactoryBean given(ObservationRegistry observationRegistry) throws Exception { - // given TestBean tb = new TestBean("tb", 99); StaticApplicationContext ac = new StaticApplicationContext(); final Scheduler scheduler = mock(Scheduler.class); SchedulerContext schedulerContext = new SchedulerContext(); BDDMockito.given(scheduler.getContext()).willReturn(schedulerContext); + return schedulerFactoryBean(observationRegistry, tb, ac, scheduler); + } + + private SchedulerFactoryBean schedulerFactoryBean(ObservationRegistry observationRegistry, TestBean tb, StaticApplicationContext ac, Scheduler scheduler) { SchedulerFactoryBean schedulerFactoryBean = new SchedulerFactoryBean() { @Override protected Scheduler createScheduler(SchedulerFactory schedulerFactory, String schedulerName) { diff --git a/spring-jms/spring-jms.gradle b/spring-jms/spring-jms.gradle index 6757f90eca2c..9af8992d7dfb 100644 --- a/spring-jms/spring-jms.gradle +++ b/spring-jms/spring-jms.gradle @@ -5,6 +5,7 @@ dependencies { api(project(":spring-core")) api(project(":spring-messaging")) api(project(":spring-tx")) + api("io.micrometer:micrometer-observation") compileOnly("jakarta.jms:jakarta.jms-api") optional(project(":spring-aop")) optional(project(":spring-context")) diff --git a/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java b/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java index 1841821a6bed..b28d7a008a4f 100644 --- a/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java +++ b/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java @@ -16,8 +16,8 @@ package org.springframework.jms.listener; -import io.micrometer.core.instrument.observation.Observation; -import io.micrometer.core.instrument.observation.ObservationRegistry; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; import jakarta.jms.Connection; import jakarta.jms.Destination; import jakarta.jms.JMSException; diff --git a/spring-tx/spring-tx.gradle b/spring-tx/spring-tx.gradle index 2d0f9958028e..fba40fb3e234 100644 --- a/spring-tx/spring-tx.gradle +++ b/spring-tx/spring-tx.gradle @@ -5,7 +5,7 @@ apply plugin: "kotlin" dependencies { api(project(":spring-beans")) api(project(":spring-core")) - api("io.micrometer:micrometer-core") + api("io.micrometer:micrometer-observation") optional(project(":spring-aop")) optional(project(":spring-context")) // for JCA, @EnableTransactionManagement optional("jakarta.ejb:jakarta.ejb-api") @@ -28,4 +28,6 @@ dependencies { testImplementation("jakarta.persistence:jakarta.persistence-api") testImplementation("io.projectreactor:reactor-test") optional("io.micrometer:micrometer-tracing-integration-test") // I don't know how to make this be in tests and testFixtures + optional("io.micrometer:micrometer-core") // I don't know how to make this be in tests and testFixtures + testImplementation("io.micrometer:micrometer-observation-test") } diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java index 01d9962e73b8..625451d8501f 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java @@ -20,8 +20,8 @@ import java.util.Properties; import java.util.concurrent.ConcurrentMap; -import io.micrometer.core.instrument.observation.Observation; -import io.micrometer.core.instrument.observation.ObservationRegistry; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; import io.vavr.control.Try; import kotlin.coroutines.Continuation; import kotlinx.coroutines.reactive.AwaitKt; diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java index b3c3f523a840..258aadfdc3e3 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java @@ -16,8 +16,8 @@ package org.springframework.transaction.support; -import io.micrometer.core.instrument.Tags; -import io.micrometer.core.instrument.observation.Observation; +import io.micrometer.observation.Observation; +import io.micrometer.observation.Tags; import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.annotation.Isolation; diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java index 911866e952c5..14dcbc72933e 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java @@ -16,8 +16,8 @@ package org.springframework.transaction.support; -import io.micrometer.core.instrument.observation.Observation; -import io.micrometer.core.instrument.observation.ObservationRegistry; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java index 84179eb0c52a..fd5057541e3d 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java @@ -16,8 +16,8 @@ package org.springframework.transaction.support; -import io.micrometer.core.instrument.docs.DocumentedObservation; -import io.micrometer.core.instrument.docs.TagKey; +import io.micrometer.observation.docs.DocumentedObservation; +import io.micrometer.observation.docs.TagKey; enum TransactionObservation implements DocumentedObservation { diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java index 8df652890dbc..36dac7dbf425 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java @@ -16,7 +16,7 @@ package org.springframework.transaction.support; -import io.micrometer.core.instrument.observation.Observation; +import io.micrometer.observation.Observation; import org.springframework.lang.Nullable; import org.springframework.transaction.TransactionDefinition; diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java index 7c4afbf30a19..ed5a6d4ffb71 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java @@ -16,7 +16,7 @@ package org.springframework.transaction.support; -import io.micrometer.core.instrument.observation.Observation; +import io.micrometer.observation.Observation; /** * A {@link Observation.TagsProvider} for Spring Transaction. diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java index 548b2060bf24..19507862a11c 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java @@ -18,8 +18,8 @@ import java.lang.reflect.UndeclaredThrowableException; -import io.micrometer.core.instrument.observation.Observation; -import io.micrometer.core.instrument.observation.ObservationRegistry; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java index 9c85686621e0..55d4509e763f 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/ObservationTransactionTemplateTests.java @@ -16,7 +16,7 @@ package org.springframework.transaction.interceptor; -import io.micrometer.core.instrument.observation.ObservationRegistry; +import io.micrometer.observation.ObservationRegistry; import org.springframework.transaction.support.TransactionCallback; import org.springframework.transaction.support.TransactionTemplate; diff --git a/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java b/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java new file mode 100644 index 000000000000..15e5c0da1f69 --- /dev/null +++ b/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java @@ -0,0 +1,115 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.support; + +import io.micrometer.core.tck.TestObservationRegistry; +import io.micrometer.observation.ObservationRegistry; +import org.junit.jupiter.api.Test; +import org.mockito.BDDMockito; + +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionManager; +import org.springframework.transaction.TransactionStatus; + +import static io.micrometer.core.tck.TestObservationRegistryAssert.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.BDDMockito.then; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; + +class ObservationPlatformTransactionManagerTests { + + TestObservationRegistry observationRegistry = TestObservationRegistry.create(); + + PlatformTransactionManager delegate = mock(PlatformTransactionManager.class); + + private static final TransactionManager transactionManager = new TransactionManager() { + + }; + + @Test + void commitAfterGetTransaction() { + ObservationPlatformTransactionManager manager = observationPlatformTransactionManager(); + + TransactionStatus transaction = manager.getTransaction(new DefaultTransactionDefinition()); + manager.commit(transaction); + + assertThatRegistryHasASingleStoppedTaggedObservation(); + } + + @Test + void rollbackAfterGetTransaction() { + ObservationPlatformTransactionManager manager = observationPlatformTransactionManager(); + + TransactionStatus transaction = manager.getTransaction(new DefaultTransactionDefinition()); + manager.rollback(transaction); + + assertThatRegistryHasASingleStoppedTaggedObservation(); + } + + @Test + void commitForNoOpRegistryDelegatesToManagerWithoutCallingObservation() { + TransactionObservationContext context = mock(TransactionObservationContext.class); + ObservationPlatformTransactionManager manager = noOpDelegateObservationPlatformTransactionManager(context); + + manager.commit(null); + + then(this.delegate).should().commit(null); + verifyNoInteractions(context); + } + + @Test + void rollbackForNoOpRegistryDelegatesToManagerWithoutCallingObservation() { + TransactionObservationContext context = mock(TransactionObservationContext.class); + ObservationPlatformTransactionManager manager = noOpDelegateObservationPlatformTransactionManager(context); + + manager.rollback(null); + + then(this.delegate).should().rollback(null); + verifyNoInteractions(context); + } + + private ObservationPlatformTransactionManager observationPlatformTransactionManager() { + TransactionObservationContext context = new TransactionObservationContext(transactionDefinition(), transactionManager); + given(this.delegate.getTransaction(BDDMockito.any())).willReturn(new SimpleTransactionStatus()); + return new ObservationPlatformTransactionManager(this.delegate, this.observationRegistry, context, new DefaultTransactionTagsProvider()); + } + + private ObservationPlatformTransactionManager noOpDelegateObservationPlatformTransactionManager(TransactionObservationContext context) { + return new ObservationPlatformTransactionManager(this.delegate, ObservationRegistry.NOOP, context, new DefaultTransactionTagsProvider()); + } + + private void assertThatRegistryHasASingleStoppedTaggedObservation() { + assertThat(observationRegistry) + .doesNotHaveAnyRemainingCurrentObservation() + .hasSingleObservationThat() + .hasLowCardinalityTag("spring.tx.isolation-level", "-1") + .hasLowCardinalityTag("spring.tx.propagation-level", "REQUIRED") + .hasLowCardinalityTag("spring.tx.read-only", "false") + .hasLowCardinalityTag("spring.tx.transaction-manager", "org.springframework.transaction.support.ObservationPlatformTransactionManagerTests$1") + .hasHighCardinalityTag("spring.tx.name", "foo") + .hasBeenStarted() + .hasBeenStopped(); + } + + private DefaultTransactionDefinition transactionDefinition() { + DefaultTransactionDefinition definition = new DefaultTransactionDefinition(); + definition.setName("foo"); + return definition; + } + +} diff --git a/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java b/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java index 10721cb8c263..5107a3868092 100644 --- a/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java +++ b/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java @@ -19,9 +19,8 @@ import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.Tag; import io.micrometer.core.instrument.Tags; -import io.micrometer.core.instrument.observation.ObservationRegistry; -import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import io.micrometer.core.tck.MeterRegistryAssert; +import io.micrometer.observation.ObservationRegistry; import io.micrometer.tracing.test.SampleTestRunner; import io.micrometer.tracing.test.reporter.BuildingBlocks; import io.micrometer.tracing.test.simple.SpansAssert; @@ -34,13 +33,13 @@ public abstract class ObservationTransactionManagerSampleTestRunner extends SampleTestRunner { public ObservationTransactionManagerSampleTestRunner() { - super(SampleRunnerConfig.builder().build(), new SimpleMeterRegistry().withTimerObservationHandler()); + super(SampleRunnerConfig.builder().build()); } @Override public SampleTestRunnerConsumer yourCode() throws Exception { return (bb, meterRegistry) -> { - T sut = given(meterRegistry); + T sut = given(getObservationRegistry()); when(sut); From 4d876b98c93165487d0a712f0265c343b5a3cd67 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 21 Mar 2022 11:58:17 +0100 Subject: [PATCH 08/15] Creates a new scope for the child observation --- ...ObservationPlatformTransactionManager.java | 30 ++++---- .../TransactionObservationContext.java | 10 --- ...vationPlatformTransactionManagerTests.java | 69 +++++++++++++++---- ...ionTransactionManagerSampleTestRunner.java | 2 +- 4 files changed, 76 insertions(+), 35 deletions(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java index 14dcbc72933e..c179f4024312 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java @@ -57,21 +57,27 @@ public TransactionStatus getTransaction(TransactionDefinition definition) throws return this.delegate.getTransaction(definition); } Observation obs = this.observationRegistry.getCurrentObservation(); - obs = obs != null ? obs : Observation.NOOP; - try (Observation.Scope scope = obs.openScope()) { - obs = TransactionObservation.TX_OBSERVATION.observation(this.observationRegistry, this.context) - .tagsProvider(this.transactionTagsProvider) - .start(); - this.context.setScope(scope); + if (obs == null || obs.isNoOp()) { + this.context.setScope(Observation.Scope.NOOP); + return this.delegate.getTransaction(definition); + } + if (log.isDebugEnabled()) { + log.debug("A previous observation is in scope - will create a child one"); + } + Observation childObs = TransactionObservation.TX_OBSERVATION.observation(this.observationRegistry, this.context) + .tagsProvider(this.transactionTagsProvider) + .start(); + this.context.setScope(childObs.openScope()); + if (log.isDebugEnabled()) { + log.debug("Child scope created"); } - this.context.setObservation(obs); try { TransactionStatus transaction = this.delegate.getTransaction(definition); this.context.setTransactionStatus(transaction); return transaction; } catch (Exception ex) { - this.context.getObservation().error(ex).stop(); + childObs.error(ex).stop(); throw ex; } } @@ -89,12 +95,12 @@ public void commit(TransactionStatus status) throws TransactionException { this.delegate.commit(status); } catch (Exception ex) { - this.context.getObservation().error(ex); + this.context.getScope().getCurrentObservation().error(ex); throw ex; } finally { + this.context.getScope().getCurrentObservation().stop(); this.context.getScope().close(); - this.context.getObservation().stop(); } } @@ -111,12 +117,12 @@ public void rollback(TransactionStatus status) throws TransactionException { this.delegate.rollback(status); } catch (Exception ex) { - this.context.getObservation().error(ex); + this.context.getScope().getCurrentObservation().error(ex); throw ex; } finally { + this.context.getScope().getCurrentObservation().stop(); this.context.getScope().close(); - this.context.getObservation().stop(); } } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java index 36dac7dbf425..0f3b3a391339 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationContext.java @@ -37,8 +37,6 @@ public class TransactionObservationContext extends Observation.Context { private TransactionStatus transactionStatus; - private Observation observation = Observation.NOOP; - private Observation.Scope scope = Observation.Scope.NOOP; public TransactionObservationContext(@Nullable TransactionDefinition transactionDefinition, TransactionManager transactionManager) { @@ -62,14 +60,6 @@ public void setTransactionStatus(TransactionStatus transactionStatus) { this.transactionStatus = transactionStatus; } - public Observation getObservation() { - return this.observation; - } - - public void setObservation(Observation observation) { - this.observation = observation; - } - public Observation.Scope getScope() { return this.scope; } diff --git a/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java b/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java index 15e5c0da1f69..d68b4398d6e1 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java @@ -17,7 +17,9 @@ package org.springframework.transaction.support; import io.micrometer.core.tck.TestObservationRegistry; +import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationRegistry; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import org.mockito.BDDMockito; @@ -41,24 +43,67 @@ class ObservationPlatformTransactionManagerTests { }; + @AfterEach + void noLeakingObservationRemaining() { + assertThat(this.observationRegistry).doesNotHaveAnyRemainingCurrentObservation(); + } + @Test - void commitAfterGetTransaction() { + void getTransactionLeavesOpenScopeWhenPreviousObservationWasPresent() { + try (Observation.Scope scope = Observation.start("parent", this.observationRegistry).openScope()) { + ObservationPlatformTransactionManager manager = observationPlatformTransactionManager(); + + manager.getTransaction(new DefaultTransactionDefinition()); + + assertThat(this.observationRegistry).doesNotHaveRemainingCurrentObservationSameAs(scope.getCurrentObservation()); + } + } + + @Test + void getTransactionDoesNothingWhenNoPreviousObservationPresent() { ObservationPlatformTransactionManager manager = observationPlatformTransactionManager(); - TransactionStatus transaction = manager.getTransaction(new DefaultTransactionDefinition()); - manager.commit(transaction); + manager.getTransaction(new DefaultTransactionDefinition()); - assertThatRegistryHasASingleStoppedTaggedObservation(); + assertThat(this.observationRegistry).doesNotHaveAnyRemainingCurrentObservation(); + } + + @Test + void commitAfterGetTransaction() { + try (Observation.Scope scope = Observation.start("parent", this.observationRegistry).openScope()) { + ObservationPlatformTransactionManager manager = observationPlatformTransactionManager(); + + // when + TransactionStatus transaction = manager.getTransaction(new DefaultTransactionDefinition()); + + // then + assertThat(this.observationRegistry).doesNotHaveRemainingCurrentObservationSameAs(scope.getCurrentObservation()); + + // when + manager.commit(transaction); + + // then + assertThatRegistryHasAStoppedTxObservation(scope); + } } @Test void rollbackAfterGetTransaction() { - ObservationPlatformTransactionManager manager = observationPlatformTransactionManager(); + try (Observation.Scope scope = Observation.start("parent", this.observationRegistry).openScope()) { + ObservationPlatformTransactionManager manager = observationPlatformTransactionManager(); + + // when + TransactionStatus transaction = manager.getTransaction(new DefaultTransactionDefinition()); + + // then + assertThat(this.observationRegistry).doesNotHaveRemainingCurrentObservationSameAs(scope.getCurrentObservation()); - TransactionStatus transaction = manager.getTransaction(new DefaultTransactionDefinition()); - manager.rollback(transaction); + // when + manager.rollback(transaction); - assertThatRegistryHasASingleStoppedTaggedObservation(); + // then + assertThatRegistryHasAStoppedTxObservation(scope); + } } @Test @@ -93,10 +138,10 @@ private ObservationPlatformTransactionManager noOpDelegateObservationPlatformTra return new ObservationPlatformTransactionManager(this.delegate, ObservationRegistry.NOOP, context, new DefaultTransactionTagsProvider()); } - private void assertThatRegistryHasASingleStoppedTaggedObservation() { - assertThat(observationRegistry) - .doesNotHaveAnyRemainingCurrentObservation() - .hasSingleObservationThat() + private void assertThatRegistryHasAStoppedTxObservation(Observation.Scope scope) { + assertThat(this.observationRegistry) + .hasRemainingCurrentObservationSameAs(scope.getCurrentObservation()) + .hasObservationWithNameEqualTo("spring.tx").that() .hasLowCardinalityTag("spring.tx.isolation-level", "-1") .hasLowCardinalityTag("spring.tx.propagation-level", "REQUIRED") .hasLowCardinalityTag("spring.tx.read-only", "false") diff --git a/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java b/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java index 5107a3868092..4b18588e99e5 100644 --- a/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java +++ b/spring-tx/src/testFixtures/java/org/springframework/transaction/testfixture/ObservationTransactionManagerSampleTestRunner.java @@ -32,7 +32,7 @@ */ public abstract class ObservationTransactionManagerSampleTestRunner extends SampleTestRunner { - public ObservationTransactionManagerSampleTestRunner() { + protected ObservationTransactionManagerSampleTestRunner() { super(SampleRunnerConfig.builder().build()); } From eff1c154d109dc68c4ead5ff8f78ce99a88f58ab Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 21 Mar 2022 12:10:43 +0100 Subject: [PATCH 09/15] Added simple tests for default transaction tags provider --- .../DefaultTransactionTagsProvider.java | 3 ++ .../DefaultTransactionTagsProviderTests.java | 54 +++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionTagsProviderTests.java diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java index 258aadfdc3e3..0427f3287d14 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java @@ -55,6 +55,9 @@ public Tags getLowCardinalityTags(TransactionObservationContext context) { @Override public Tags getHighCardinalityTags(TransactionObservationContext context) { + if (!context.getTransactionStatus().isNewTransaction()) { + return Tags.empty(); + } TransactionDefinition transactionDefinition = context.getTransactionDefinition(); if (StringUtils.hasText(transactionDefinition.getName())) { return Tags.of(TransactionObservation.HighCardinalityTags.NAME.of(transactionDefinition.getName())); diff --git a/spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionTagsProviderTests.java b/spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionTagsProviderTests.java new file mode 100644 index 000000000000..a10cad108416 --- /dev/null +++ b/spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionTagsProviderTests.java @@ -0,0 +1,54 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.support; + +import io.micrometer.observation.Tags; +import org.junit.jupiter.api.Test; + +import org.springframework.transaction.TransactionManager; +import org.springframework.transaction.TransactionStatus; + +import static org.assertj.core.api.BDDAssertions.then; + +class DefaultTransactionTagsProviderTests { + + @Test + void noTagsForContinuedTransaction() { + TransactionStatus transactionStatus = new SimpleTransactionStatus(false); + TransactionObservationContext context = new TransactionObservationContext(null, new TransactionManager() { + + }); + context.setTransactionStatus(transactionStatus); + + then(new DefaultTransactionTagsProvider().getLowCardinalityTags(context)).isSameAs(Tags.empty()); + then(new DefaultTransactionTagsProvider().getHighCardinalityTags(context)).isSameAs(Tags.empty()); + } + + @Test + void tagsForNewTransaction() { + TransactionStatus transactionStatus = new SimpleTransactionStatus(true); + DefaultTransactionDefinition definition = new DefaultTransactionDefinition(); + definition.setName("foo"); + TransactionObservationContext context = new TransactionObservationContext(definition, new TransactionManager() { + + }); + context.setTransactionStatus(transactionStatus); + + then(new DefaultTransactionTagsProvider().getLowCardinalityTags(context)).isNotSameAs(Tags.empty()); + then(new DefaultTransactionTagsProvider().getHighCardinalityTags(context)).isNotSameAs(Tags.empty()); + } +} From d960e2e50edb484a75eb1a94d55b366361ed00f4 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 21 Mar 2022 12:20:08 +0100 Subject: [PATCH 10/15] Creates objects only when necessary --- .../scheduling/quartz/SchedulerAccessor.java | 16 +++++++++------- ...AbstractPollingMessageListenerContainer.java | 13 ++++++++----- .../support/TransactionTemplate.java | 17 ++++++++++------- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java index a272e8777d81..020723212f1b 100644 --- a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java +++ b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java @@ -224,11 +224,13 @@ public void setTagsProvider(TransactionTagsProvider tagsProvider) { protected void registerJobsAndTriggers() throws SchedulerException { TransactionStatus transactionStatus = null; TransactionDefinition definition = TransactionDefinition.withDefaults(); - ObservationPlatformTransactionManager observationPlatformTransactionManager = null; - if (this.transactionManager != null) { - TransactionObservationContext context = new TransactionObservationContext(definition, this.transactionManager); - observationPlatformTransactionManager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); - transactionStatus = observationPlatformTransactionManager.getTransaction(definition); + PlatformTransactionManager manager = this.transactionManager; + if (manager != null) { + if (!this.observationRegistry.isNoOp()) { + TransactionObservationContext context = new TransactionObservationContext(definition, manager); + manager = new ObservationPlatformTransactionManager(manager, this.observationRegistry, context, this.tagsProvider); + } + transactionStatus = manager.getTransaction(definition); } try { @@ -271,7 +273,7 @@ protected void registerJobsAndTriggers() throws SchedulerException { catch (Throwable ex) { if (transactionStatus != null) { try { - observationPlatformTransactionManager.rollback(transactionStatus); + manager.rollback(transactionStatus); } catch (TransactionException tex) { logger.error("Job registration exception overridden by rollback exception", ex); @@ -288,7 +290,7 @@ protected void registerJobsAndTriggers() throws SchedulerException { } if (transactionStatus != null) { - observationPlatformTransactionManager.commit(transactionStatus); + manager.commit(transactionStatus); } } diff --git a/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java b/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java index b28d7a008a4f..9c17afcdf342 100644 --- a/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java +++ b/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java @@ -256,20 +256,23 @@ protected boolean receiveAndExecute( throws JMSException { if (this.transactionManager != null) { - TransactionObservationContext context = new TransactionObservationContext(this.transactionDefinition, this.transactionManager); - ObservationPlatformTransactionManager observationPlatformTransactionManager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); + PlatformTransactionManager manager = this.transactionManager; + if (!this.observationRegistry.isNoOp()) { + TransactionObservationContext context = new TransactionObservationContext(this.transactionDefinition, this.transactionManager); + manager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); + } // Execute receive within transaction. - TransactionStatus status = observationPlatformTransactionManager.getTransaction(this.transactionDefinition); + TransactionStatus status = manager.getTransaction(this.transactionDefinition); boolean messageReceived; try { messageReceived = doReceiveAndExecute(invoker, session, consumer, status); } catch (JMSException | RuntimeException | Error ex) { - rollbackOnException(observationPlatformTransactionManager, status, ex); + rollbackOnException(manager, status, ex); throw ex; } try { - observationPlatformTransactionManager.commit(status); + manager.commit(status); } catch (TransactionException ex) { // Propagate transaction system exceptions as infrastructure problems. diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java index 19507862a11c..a7d6ccd0a7bd 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java @@ -148,24 +148,27 @@ public T execute(TransactionCallback action) throws TransactionException return ((CallbackPreferringPlatformTransactionManager) this.transactionManager).execute(this, action); } else { - TransactionObservationContext context = new TransactionObservationContext(this, this.transactionManager); - ObservationPlatformTransactionManager observationPlatformTransactionManager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); - TransactionStatus status = observationPlatformTransactionManager.getTransaction(this); + PlatformTransactionManager manager = this.transactionManager; + if (!this.observationRegistry.isNoOp()) { + TransactionObservationContext context = new TransactionObservationContext(this, this.transactionManager); + manager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); + } + TransactionStatus status = manager.getTransaction(this); T result; try { result = action.doInTransaction(status); } catch (RuntimeException | Error ex) { // Transactional code threw application exception -> rollback - rollbackOnException(observationPlatformTransactionManager, status, ex); + rollbackOnException(manager, status, ex); throw ex; } catch (Throwable ex) { // Transactional code threw unexpected exception -> rollback - rollbackOnException(observationPlatformTransactionManager, status, ex); + rollbackOnException(manager, status, ex); throw new UndeclaredThrowableException(ex, "TransactionCallback threw undeclared checked exception"); } - observationPlatformTransactionManager.commit(status); + manager.commit(status); return result; } } @@ -177,7 +180,7 @@ public T execute(TransactionCallback action) throws TransactionException * @param ex the thrown application exception or error * @throws TransactionException in case of a rollback error */ - private void rollbackOnException(ObservationPlatformTransactionManager observationPlatformTransactionManager, TransactionStatus status, Throwable ex) throws TransactionException { + private void rollbackOnException(PlatformTransactionManager observationPlatformTransactionManager, TransactionStatus status, Throwable ex) throws TransactionException { Assert.state(this.transactionManager != null, "No PlatformTransactionManager set"); logger.debug("Initiating transaction rollback on application exception", ex); From 51e8094b154f186232dff7d9329805c908a96c2d Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 21 Mar 2022 12:28:07 +0100 Subject: [PATCH 11/15] Ensures that delegates are called --- .../support/TransactionTemplate.java | 6 +++--- ...vationPlatformTransactionManagerTests.java | 20 ++++++++++++++----- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java index a7d6ccd0a7bd..d27e5eb30a95 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java @@ -175,17 +175,17 @@ public T execute(TransactionCallback action) throws TransactionException /** * Perform a rollback, handling rollback exceptions properly. - * @param observationPlatformTransactionManager observation platform manager + * @param platformTransactionManager platform transaction manager * @param status object representing the transaction * @param ex the thrown application exception or error * @throws TransactionException in case of a rollback error */ - private void rollbackOnException(PlatformTransactionManager observationPlatformTransactionManager, TransactionStatus status, Throwable ex) throws TransactionException { + private void rollbackOnException(PlatformTransactionManager platformTransactionManager, TransactionStatus status, Throwable ex) throws TransactionException { Assert.state(this.transactionManager != null, "No PlatformTransactionManager set"); logger.debug("Initiating transaction rollback on application exception", ex); try { - observationPlatformTransactionManager.rollback(status); + platformTransactionManager.rollback(status); } catch (TransactionSystemException ex2) { logger.error("Application exception overridden by rollback exception", ex); diff --git a/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java b/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java index d68b4398d6e1..380ea1555d80 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java @@ -21,13 +21,13 @@ import io.micrometer.observation.ObservationRegistry; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; -import org.mockito.BDDMockito; import org.springframework.transaction.PlatformTransactionManager; import org.springframework.transaction.TransactionManager; import org.springframework.transaction.TransactionStatus; import static io.micrometer.core.tck.TestObservationRegistryAssert.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.then; import static org.mockito.Mockito.mock; @@ -55,17 +55,21 @@ void getTransactionLeavesOpenScopeWhenPreviousObservationWasPresent() { manager.getTransaction(new DefaultTransactionDefinition()); - assertThat(this.observationRegistry).doesNotHaveRemainingCurrentObservationSameAs(scope.getCurrentObservation()); + assertThat(this.observationRegistry) + .hasRemainingCurrentObservation() + .doesNotHaveRemainingCurrentObservationSameAs(scope.getCurrentObservation()); + then(this.delegate).should().getTransaction(any()); } } @Test - void getTransactionDoesNothingWhenNoPreviousObservationPresent() { + void getTransactionJustDelegatesWhenNoPreviousObservationPresent() { ObservationPlatformTransactionManager manager = observationPlatformTransactionManager(); manager.getTransaction(new DefaultTransactionDefinition()); assertThat(this.observationRegistry).doesNotHaveAnyRemainingCurrentObservation(); + then(this.delegate).should().getTransaction(any()); } @Test @@ -77,13 +81,17 @@ void commitAfterGetTransaction() { TransactionStatus transaction = manager.getTransaction(new DefaultTransactionDefinition()); // then - assertThat(this.observationRegistry).doesNotHaveRemainingCurrentObservationSameAs(scope.getCurrentObservation()); + assertThat(this.observationRegistry) + .hasRemainingCurrentObservation() + .doesNotHaveRemainingCurrentObservationSameAs(scope.getCurrentObservation()); + then(this.delegate).should().getTransaction(any()); // when manager.commit(transaction); // then assertThatRegistryHasAStoppedTxObservation(scope); + then(this.delegate).should().commit(any()); } } @@ -94,6 +102,7 @@ void rollbackAfterGetTransaction() { // when TransactionStatus transaction = manager.getTransaction(new DefaultTransactionDefinition()); + then(this.delegate).should().getTransaction(any()); // then assertThat(this.observationRegistry).doesNotHaveRemainingCurrentObservationSameAs(scope.getCurrentObservation()); @@ -103,6 +112,7 @@ void rollbackAfterGetTransaction() { // then assertThatRegistryHasAStoppedTxObservation(scope); + then(this.delegate).should().rollback(any()); } } @@ -130,7 +140,7 @@ void rollbackForNoOpRegistryDelegatesToManagerWithoutCallingObservation() { private ObservationPlatformTransactionManager observationPlatformTransactionManager() { TransactionObservationContext context = new TransactionObservationContext(transactionDefinition(), transactionManager); - given(this.delegate.getTransaction(BDDMockito.any())).willReturn(new SimpleTransactionStatus()); + given(this.delegate.getTransaction(any())).willReturn(new SimpleTransactionStatus()); return new ObservationPlatformTransactionManager(this.delegate, this.observationRegistry, context, new DefaultTransactionTagsProvider()); } From 38fe0d94ee6eb4c9b9e34f375e69237abbbb4f5d Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 21 Mar 2022 13:15:12 +0100 Subject: [PATCH 12/15] Wraps aspect support only when observation registry is not noop --- .../transaction/interceptor/TransactionAspectSupport.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java index 625451d8501f..41fa74fd56a9 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java @@ -611,7 +611,7 @@ public String getName() { TransactionStatus status = null; if (txAttr != null) { if (tm != null) { - if (this.transactionManager instanceof PlatformTransactionManager) { + if (this.transactionManager instanceof PlatformTransactionManager && !this.observationRegistry.isNoOp()) { TransactionObservationContext context = new TransactionObservationContext(txAttr, this.transactionManager); tm = new ObservationPlatformTransactionManager((PlatformTransactionManager) this.transactionManager, this.observationRegistry, context, this.tagsProvider); } From 5667623ce41dcbb7ffd078f0bd62c56ce8cb2b67 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Tue, 22 Mar 2022 10:05:45 +0100 Subject: [PATCH 13/15] Makes a copy of resources and synchronizations --- .../support/TransactionThreadLocalAccessor.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java index f38ad8d8b648..a351439b7acf 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java @@ -16,6 +16,8 @@ package org.springframework.transaction.support; +import java.util.HashMap; +import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -65,7 +67,8 @@ public void restoreValues(ContextContainer container) { @Override public void resetValues(ContextContainer container) { - if (Boolean.parseBoolean(String.valueOf(container.get(PREVIOUS_TRANSACTION_KEY)))) { + Object previousTransaction = container.get(PREVIOUS_TRANSACTION_KEY); + if (previousTransaction != null && Boolean.parseBoolean(String.valueOf(previousTransaction.toString()))) { if (log.isDebugEnabled()) { log.debug("There was a transaction when context propagation happened. Will not reset the synchronization manager."); } @@ -105,8 +108,8 @@ private static TransactionSynchronizationManagerHolder create() { } private void putInScope() { - TransactionSynchronizationManager.resources.set(this.resources); - TransactionSynchronizationManager.synchronizations.set(this.synchronizations); + TransactionSynchronizationManager.resources.set(new HashMap<>(this.resources)); + TransactionSynchronizationManager.synchronizations.set(new HashSet<>(this.synchronizations)); TransactionSynchronizationManager.currentTransactionName.set(this.currentTransactionName); TransactionSynchronizationManager.currentTransactionReadOnly.set(this.currentTransactionReadOnly); TransactionSynchronizationManager.currentTransactionIsolationLevel.set(this.currentTransactionIsolationLevel); From 9a5670864bcab1d4ac2f7631d822f5a6d032e94a Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Thu, 24 Mar 2022 14:50:05 +0100 Subject: [PATCH 14/15] Removing TransactionThreadLocalAccessor after having talked to M. Paluch we came to the conslusion that such transaction propagation between threads can cause concurrency issues. We might corrupt / close / change the transaction in one thread and accidentaly break it in another one. If the problem that we're trying to solve is to allow transactions in tools like e.g. Resilience4j, we suggest creating a new transaction inside the lambda passed to such a tool. --- .../TransactionThreadLocalAccessor.java | 119 ------------------ ...ter.contextpropagation.ThreadLocalAccessor | 1 - 2 files changed, 120 deletions(-) delete mode 100644 spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java delete mode 100644 spring-tx/src/main/resources/META-INF/services/io.micrometer.contextpropagation.ThreadLocalAccessor diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java deleted file mode 100644 index a351439b7acf..000000000000 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionThreadLocalAccessor.java +++ /dev/null @@ -1,119 +0,0 @@ -/* - * Copyright 2002-2022 the original author or authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.transaction.support; - -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; - -import io.micrometer.contextpropagation.ContextContainer; -import io.micrometer.contextpropagation.ThreadLocalAccessor; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; - -import org.springframework.lang.Nullable; -import org.springframework.util.StringUtils; - -/** - * A {@link ThreadLocalAccessor} to put and restore current transactions via {@link TransactionSynchronizationManager}. - * - * @author Marcin Grzejszczak - * @since 6.0.0 - */ -public final class TransactionThreadLocalAccessor implements ThreadLocalAccessor { - - private static final Log log = LogFactory.getLog(TransactionThreadLocalAccessor.class); - - private static final String KEY = TransactionSynchronizationManagerHolder.class.getName(); - - private static final String PREVIOUS_TRANSACTION_KEY = TransactionSynchronizationManagerHolder.class.getName() + "_PREVIOUS_TRANSACTION_PRESENT"; - - @Override - public void captureValues(ContextContainer container) { - TransactionSynchronizationManagerHolder value = TransactionSynchronizationManagerHolder.create(); - container.put(KEY, value); - } - - @Override - public void restoreValues(ContextContainer container) { - String transactionName = TransactionSynchronizationManager.getCurrentTransactionName(); - if (StringUtils.hasText(transactionName)) { - container.put(PREVIOUS_TRANSACTION_KEY, true); - if (log.isDebugEnabled()) { - log.debug("A transaction with name [" + transactionName + "] has already been set in this thread. We don't want to override it. Will not update the synchronization manager with entries from the container"); - } - return; - } - if (container.containsKey(KEY)) { - TransactionSynchronizationManagerHolder holder = container.get(KEY); - holder.putInScope(); - } - } - - @Override - public void resetValues(ContextContainer container) { - Object previousTransaction = container.get(PREVIOUS_TRANSACTION_KEY); - if (previousTransaction != null && Boolean.parseBoolean(String.valueOf(previousTransaction.toString()))) { - if (log.isDebugEnabled()) { - log.debug("There was a transaction when context propagation happened. Will not reset the synchronization manager."); - } - return; - } - TransactionSynchronizationManager.clear(); - } - - static final class TransactionSynchronizationManagerHolder { - - private final Map resources; - - @Nullable - private final Set synchronizations; - - @Nullable - private final String currentTransactionName; - - private final Boolean currentTransactionReadOnly; - - @Nullable - private final Integer currentTransactionIsolationLevel; - - private final Boolean actualTransactionActive; - - private TransactionSynchronizationManagerHolder(Map resources, @Nullable Set synchronizations, @Nullable String currentTransactionName, Boolean currentTransactionReadOnly, @Nullable Integer currentTransactionIsolationLevel, Boolean actualTransactionActive) { - this.resources = resources; - this.synchronizations = synchronizations; - this.currentTransactionName = currentTransactionName; - this.currentTransactionReadOnly = currentTransactionReadOnly; - this.currentTransactionIsolationLevel = currentTransactionIsolationLevel; - this.actualTransactionActive = actualTransactionActive; - } - - private static TransactionSynchronizationManagerHolder create() { - return new TransactionSynchronizationManagerHolder(TransactionSynchronizationManager.resources.get(), TransactionSynchronizationManager.synchronizations.get(), TransactionSynchronizationManager.currentTransactionName.get(), TransactionSynchronizationManager.currentTransactionReadOnly.get(), TransactionSynchronizationManager.currentTransactionIsolationLevel.get(), TransactionSynchronizationManager.actualTransactionActive.get()); - } - - private void putInScope() { - TransactionSynchronizationManager.resources.set(new HashMap<>(this.resources)); - TransactionSynchronizationManager.synchronizations.set(new HashSet<>(this.synchronizations)); - TransactionSynchronizationManager.currentTransactionName.set(this.currentTransactionName); - TransactionSynchronizationManager.currentTransactionReadOnly.set(this.currentTransactionReadOnly); - TransactionSynchronizationManager.currentTransactionIsolationLevel.set(this.currentTransactionIsolationLevel); - TransactionSynchronizationManager.actualTransactionActive.set(this.actualTransactionActive); - } - } -} diff --git a/spring-tx/src/main/resources/META-INF/services/io.micrometer.contextpropagation.ThreadLocalAccessor b/spring-tx/src/main/resources/META-INF/services/io.micrometer.contextpropagation.ThreadLocalAccessor deleted file mode 100644 index 0913a378225d..000000000000 --- a/spring-tx/src/main/resources/META-INF/services/io.micrometer.contextpropagation.ThreadLocalAccessor +++ /dev/null @@ -1 +0,0 @@ -org.springframework.transaction.support.TransactionThreadLocalAccessor From 2567af61625bbf50b029540b3a587be341aeffa4 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 6 Feb 2023 16:56:14 +0100 Subject: [PATCH 15/15] Updated to the latest versions --- .../spring-context-support.gradle | 2 +- .../scheduling/quartz/SchedulerAccessor.java | 18 ++++---- ...stractPollingMessageListenerContainer.java | 18 ++++---- spring-tx/spring-tx.gradle | 2 +- .../interceptor/TransactionAspectSupport.java | 18 ++++---- ...aultTransactionObservationConvention.java} | 44 ++++++++++++------- ...ObservationPlatformTransactionManager.java | 17 ++++--- .../support/TransactionObservation.java | 40 ++++++++--------- ... => TransactionObservationConvention.java} | 6 +-- .../support/TransactionTemplate.java | 14 +++--- ...ransactionObservationConventionTests.java} | 12 ++--- ...vationPlatformTransactionManagerTests.java | 18 ++++---- 12 files changed, 104 insertions(+), 105 deletions(-) rename spring-tx/src/main/java/org/springframework/transaction/support/{DefaultTransactionTagsProvider.java => DefaultTransactionObservationConvention.java} (57%) rename spring-tx/src/main/java/org/springframework/transaction/support/{TransactionTagsProvider.java => TransactionObservationConvention.java} (76%) rename spring-tx/src/test/java/org/springframework/transaction/support/{DefaultTransactionTagsProviderTests.java => DefaultTransactionObservationConventionTests.java} (73%) diff --git a/spring-context-support/spring-context-support.gradle b/spring-context-support/spring-context-support.gradle index bb73e60e50dc..b49de7177c3e 100644 --- a/spring-context-support/spring-context-support.gradle +++ b/spring-context-support/spring-context-support.gradle @@ -27,5 +27,5 @@ dependencies { testFixturesApi("org.junit.jupiter:junit-jupiter-api") testFixturesImplementation("org.assertj:assertj-core") testFixturesImplementation("org.mockito:mockito-core") - testImplementation("io.micrometer:micrometer-tracing-integration-test") + testImplementation("io.micrometer:micrometer-tracing-integration-test:1.0.2-SNAPSHOT") } diff --git a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java index 020723212f1b..6efa2bb76e1d 100644 --- a/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java +++ b/spring-context-support/src/main/java/org/springframework/scheduling/quartz/SchedulerAccessor.java @@ -21,7 +21,6 @@ import java.util.List; import java.util.Map; -import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationRegistry; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -45,10 +44,10 @@ import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.TransactionException; import org.springframework.transaction.TransactionStatus; -import org.springframework.transaction.support.DefaultTransactionTagsProvider; +import org.springframework.transaction.support.DefaultTransactionObservationConvention; import org.springframework.transaction.support.ObservationPlatformTransactionManager; import org.springframework.transaction.support.TransactionObservationContext; -import org.springframework.transaction.support.TransactionTagsProvider; +import org.springframework.transaction.support.TransactionObservationConvention; /** * Common base class for accessing a Quartz Scheduler, i.e. for registering jobs, @@ -63,7 +62,7 @@ * @author Stephane Nicoll * @since 2.5.6 */ -public abstract class SchedulerAccessor implements ResourceLoaderAware, Observation.TagsProviderAware { +public abstract class SchedulerAccessor implements ResourceLoaderAware { protected final Log logger = LogFactory.getLog(getClass()); @@ -98,7 +97,7 @@ public abstract class SchedulerAccessor implements ResourceLoaderAware, Observat private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; - private TransactionTagsProvider tagsProvider = new DefaultTransactionTagsProvider(); + private TransactionObservationConvention observationConvention = new DefaultTransactionObservationConvention(); /** @@ -213,9 +212,8 @@ public void setObservationRegistry(ObservationRegistry observationRegistry) { this.observationRegistry = observationRegistry; } - @Override - public void setTagsProvider(TransactionTagsProvider tagsProvider) { - this.tagsProvider = tagsProvider; + public void setObservationConvention(TransactionObservationConvention observationConvention) { + this.observationConvention = observationConvention; } /** @@ -226,9 +224,9 @@ protected void registerJobsAndTriggers() throws SchedulerException { TransactionDefinition definition = TransactionDefinition.withDefaults(); PlatformTransactionManager manager = this.transactionManager; if (manager != null) { - if (!this.observationRegistry.isNoOp()) { + if (!this.observationRegistry.isNoop()) { TransactionObservationContext context = new TransactionObservationContext(definition, manager); - manager = new ObservationPlatformTransactionManager(manager, this.observationRegistry, context, this.tagsProvider); + manager = new ObservationPlatformTransactionManager(manager, this.observationRegistry, context, this.observationConvention); } transactionStatus = manager.getTransaction(definition); } diff --git a/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java b/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java index 9c17afcdf342..4658037699ea 100644 --- a/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java +++ b/spring-jms/src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java @@ -16,7 +16,6 @@ package org.springframework.jms.listener; -import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationRegistry; import jakarta.jms.Connection; import jakarta.jms.Destination; @@ -34,13 +33,13 @@ import org.springframework.transaction.TransactionException; import org.springframework.transaction.TransactionStatus; import org.springframework.transaction.support.DefaultTransactionDefinition; -import org.springframework.transaction.support.DefaultTransactionTagsProvider; +import org.springframework.transaction.support.DefaultTransactionObservationConvention; import org.springframework.transaction.support.ObservationPlatformTransactionManager; import org.springframework.transaction.support.ResourceTransactionManager; import org.springframework.transaction.support.TransactionObservationContext; +import org.springframework.transaction.support.TransactionObservationConvention; import org.springframework.transaction.support.TransactionSynchronizationManager; import org.springframework.transaction.support.TransactionSynchronizationUtils; -import org.springframework.transaction.support.TransactionTagsProvider; import org.springframework.util.Assert; /** @@ -81,7 +80,7 @@ * @see #receiveAndExecute * @see #setTransactionManager */ -public abstract class AbstractPollingMessageListenerContainer extends AbstractMessageListenerContainer implements Observation.TagsProviderAware { +public abstract class AbstractPollingMessageListenerContainer extends AbstractMessageListenerContainer { /** * The default receive timeout: 1000 ms = 1 second. @@ -103,7 +102,7 @@ public abstract class AbstractPollingMessageListenerContainer extends AbstractMe private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; - private TransactionTagsProvider tagsProvider = new DefaultTransactionTagsProvider(); + private TransactionObservationConvention observationConvention = new DefaultTransactionObservationConvention(); @Override @@ -197,9 +196,8 @@ public void setObservationRegistry(ObservationRegistry observationRegistry) { this.observationRegistry = observationRegistry; } - @Override - public void setTagsProvider(TransactionTagsProvider tagsProvider) { - this.tagsProvider = tagsProvider; + public void setObservationConvention(TransactionObservationConvention observationConvention) { + this.observationConvention = observationConvention; } @Override @@ -257,9 +255,9 @@ protected boolean receiveAndExecute( if (this.transactionManager != null) { PlatformTransactionManager manager = this.transactionManager; - if (!this.observationRegistry.isNoOp()) { + if (!this.observationRegistry.isNoop()) { TransactionObservationContext context = new TransactionObservationContext(this.transactionDefinition, this.transactionManager); - manager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); + manager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.observationConvention); } // Execute receive within transaction. TransactionStatus status = manager.getTransaction(this.transactionDefinition); diff --git a/spring-tx/spring-tx.gradle b/spring-tx/spring-tx.gradle index fba40fb3e234..e6f1824b2368 100644 --- a/spring-tx/spring-tx.gradle +++ b/spring-tx/spring-tx.gradle @@ -27,7 +27,7 @@ dependencies { testImplementation("org.apache.groovy:groovy") testImplementation("jakarta.persistence:jakarta.persistence-api") testImplementation("io.projectreactor:reactor-test") - optional("io.micrometer:micrometer-tracing-integration-test") // I don't know how to make this be in tests and testFixtures + optional("io.micrometer:micrometer-tracing-integration-test:1.0.2-SNAPSHOT") // I don't know how to make this be in tests and testFixtures optional("io.micrometer:micrometer-core") // I don't know how to make this be in tests and testFixtures testImplementation("io.micrometer:micrometer-observation-test") } diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java index 41fa74fd56a9..17ad4e62a249 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java @@ -20,7 +20,6 @@ import java.util.Properties; import java.util.concurrent.ConcurrentMap; -import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationRegistry; import io.vavr.control.Try; import kotlin.coroutines.Continuation; @@ -52,10 +51,10 @@ import org.springframework.transaction.TransactionSystemException; import org.springframework.transaction.reactive.TransactionContextManager; import org.springframework.transaction.support.CallbackPreferringPlatformTransactionManager; -import org.springframework.transaction.support.DefaultTransactionTagsProvider; +import org.springframework.transaction.support.DefaultTransactionObservationConvention; import org.springframework.transaction.support.ObservationPlatformTransactionManager; import org.springframework.transaction.support.TransactionObservationContext; -import org.springframework.transaction.support.TransactionTagsProvider; +import org.springframework.transaction.support.TransactionObservationConvention; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.ConcurrentReferenceHashMap; @@ -95,7 +94,7 @@ * @see #setTransactionAttributes * @see #setTransactionAttributeSource */ -public abstract class TransactionAspectSupport implements BeanFactoryAware, InitializingBean, Observation.TagsProviderAware { +public abstract class TransactionAspectSupport implements BeanFactoryAware, InitializingBean { // NOTE: This class must not implement Serializable because it serves as base // class for AspectJ aspects (which are not allowed to implement Serializable)! @@ -187,7 +186,7 @@ public static TransactionStatus currentTransactionStatus() throws NoTransactionE private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; - private TransactionTagsProvider tagsProvider = new DefaultTransactionTagsProvider(); + private TransactionObservationConvention observationConvention = new DefaultTransactionObservationConvention(); private final ConcurrentMap transactionManagerCache = new ConcurrentReferenceHashMap<>(4); @@ -317,9 +316,8 @@ public void setObservationRegistry(ObservationRegistry observationRegistry) { this.observationRegistry = observationRegistry; } - @Override - public void setTagsProvider(TransactionTagsProvider tagsProvider) { - this.tagsProvider = tagsProvider; + public void setObservationConvention(TransactionObservationConvention observationConvention) { + this.observationConvention = observationConvention; } /** @@ -611,9 +609,9 @@ public String getName() { TransactionStatus status = null; if (txAttr != null) { if (tm != null) { - if (this.transactionManager instanceof PlatformTransactionManager && !this.observationRegistry.isNoOp()) { + if (this.transactionManager instanceof PlatformTransactionManager && !this.observationRegistry.isNoop()) { TransactionObservationContext context = new TransactionObservationContext(txAttr, this.transactionManager); - tm = new ObservationPlatformTransactionManager((PlatformTransactionManager) this.transactionManager, this.observationRegistry, context, this.tagsProvider); + tm = new ObservationPlatformTransactionManager((PlatformTransactionManager) this.transactionManager, this.observationRegistry, context, this.observationConvention); } status = tm.getTransaction(txAttr); } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionObservationConvention.java similarity index 57% rename from spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java rename to spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionObservationConvention.java index 0427f3287d14..63e8071726ac 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionTagsProvider.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionObservationConvention.java @@ -16,8 +16,8 @@ package org.springframework.transaction.support; +import io.micrometer.common.KeyValues; import io.micrometer.observation.Observation; -import io.micrometer.observation.Tags; import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.annotation.Isolation; @@ -26,43 +26,43 @@ import org.springframework.util.StringUtils; /** - * Default implementation for {@link TransactionTagsProvider}. + * Default implementation for {@link TransactionObservationConvention}. * * @author Marcin Grzejszczak * @since 6.0.0 */ -public class DefaultTransactionTagsProvider implements TransactionTagsProvider { +public class DefaultTransactionObservationConvention implements TransactionObservationConvention { private static final Propagation[] PROPAGATIONS = Propagation.values(); private static final Isolation[] ISOLATIONS = Isolation.values(); @Override - public Tags getLowCardinalityTags(TransactionObservationContext context) { - if (!context.getTransactionStatus().isNewTransaction()) { - return Tags.empty(); + public KeyValues getLowCardinalityKeyValues(TransactionObservationContext context) { + if (context.getTransactionStatus() == null || !context.getTransactionStatus().isNewTransaction()) { + return KeyValues.empty(); } TransactionDefinition transactionDefinition = context.getTransactionDefinition(); - Tags tags = Tags.empty(); + KeyValues tags = KeyValues.empty(); if (transactionDefinition.getTimeout() > 0) { - tags = tags.and(TransactionObservation.LowCardinalityTags.TIMEOUT.of(String.valueOf(transactionDefinition.getTimeout()))); + tags = tags.and(TransactionObservation.LowCardinalityKeyNames.TIMEOUT.withValue(String.valueOf(transactionDefinition.getTimeout()))); } - return tags.and(TransactionObservation.LowCardinalityTags.TRANSACTION_MANAGER.of(ClassUtils.getQualifiedName(context.getTransactionManagerClass())), - TransactionObservation.LowCardinalityTags.READ_ONLY.of(String.valueOf(transactionDefinition.isReadOnly())), - TransactionObservation.LowCardinalityTags.PROPAGATION_LEVEL.of(propagationLevel(transactionDefinition)), - TransactionObservation.LowCardinalityTags.ISOLATION_LEVEL.of(isolationLevel(transactionDefinition))); + return tags.and(TransactionObservation.LowCardinalityKeyNames.TRANSACTION_MANAGER.withValue(ClassUtils.getQualifiedName(context.getTransactionManagerClass())), + TransactionObservation.LowCardinalityKeyNames.READ_ONLY.withValue(String.valueOf(transactionDefinition.isReadOnly())), + TransactionObservation.LowCardinalityKeyNames.PROPAGATION_LEVEL.withValue(propagationLevel(transactionDefinition)), + TransactionObservation.LowCardinalityKeyNames.ISOLATION_LEVEL.withValue(isolationLevel(transactionDefinition))); } @Override - public Tags getHighCardinalityTags(TransactionObservationContext context) { - if (!context.getTransactionStatus().isNewTransaction()) { - return Tags.empty(); + public KeyValues getHighCardinalityKeyValues(TransactionObservationContext context) { + if (context.getTransactionStatus() == null || !context.getTransactionStatus().isNewTransaction()) { + return KeyValues.empty(); } TransactionDefinition transactionDefinition = context.getTransactionDefinition(); if (StringUtils.hasText(transactionDefinition.getName())) { - return Tags.of(TransactionObservation.HighCardinalityTags.NAME.of(transactionDefinition.getName())); + return KeyValues.of(TransactionObservation.HighCardinalityKeyNames.NAME.withValue(transactionDefinition.getName())); } - return Tags.empty(); + return KeyValues.empty(); } @Override @@ -70,6 +70,16 @@ public boolean supportsContext(Observation.Context context) { return context instanceof TransactionObservationContext; } + @Override + public String getName() { + return "spring.tx"; + } + + @Override + public String getContextualName(TransactionObservationContext context) { + return "tx"; + } + private static String propagationLevel(TransactionDefinition def) { if (def.getPropagationBehavior() < 0 || def.getPropagationBehavior() >= PROPAGATIONS.length) { return String.valueOf(def.getPropagationBehavior()); diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java index c179f4024312..906390014b0e 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/ObservationPlatformTransactionManager.java @@ -42,30 +42,29 @@ public final class ObservationPlatformTransactionManager implements PlatformTran private final TransactionObservationContext context; - private final TransactionTagsProvider transactionTagsProvider; + private final TransactionObservationConvention transactionObservationConvention; - public ObservationPlatformTransactionManager(PlatformTransactionManager delegate, ObservationRegistry observationRegistry, TransactionObservationContext context, TransactionTagsProvider transactionTagsProvider) { + public ObservationPlatformTransactionManager(PlatformTransactionManager delegate, ObservationRegistry observationRegistry, TransactionObservationContext context, TransactionObservationConvention transactionObservationConvention) { this.delegate = delegate; this.observationRegistry = observationRegistry; this.context = context; - this.transactionTagsProvider = transactionTagsProvider; + this.transactionObservationConvention = transactionObservationConvention; } @Override public TransactionStatus getTransaction(TransactionDefinition definition) throws TransactionException { - if (this.observationRegistry.isNoOp()) { + if (this.observationRegistry.isNoop()) { return this.delegate.getTransaction(definition); } Observation obs = this.observationRegistry.getCurrentObservation(); - if (obs == null || obs.isNoOp()) { + if (obs == null || obs.isNoop()) { this.context.setScope(Observation.Scope.NOOP); return this.delegate.getTransaction(definition); } if (log.isDebugEnabled()) { log.debug("A previous observation is in scope - will create a child one"); } - Observation childObs = TransactionObservation.TX_OBSERVATION.observation(this.observationRegistry, this.context) - .tagsProvider(this.transactionTagsProvider) + Observation childObs = TransactionObservation.TX_OBSERVATION.observation(this.transactionObservationConvention, new DefaultTransactionObservationConvention(), () -> this.context, this.observationRegistry) .start(); this.context.setScope(childObs.openScope()); if (log.isDebugEnabled()) { @@ -84,7 +83,7 @@ public TransactionStatus getTransaction(TransactionDefinition definition) throws @Override public void commit(TransactionStatus status) throws TransactionException { - if (this.observationRegistry.isNoOp()) { + if (this.observationRegistry.isNoop()) { this.delegate.commit(status); return; } @@ -106,7 +105,7 @@ public void commit(TransactionStatus status) throws TransactionException { @Override public void rollback(TransactionStatus status) throws TransactionException { - if (this.observationRegistry.isNoOp()) { + if (this.observationRegistry.isNoop()) { this.delegate.rollback(status); return; } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java index fd5057541e3d..8d5dcbb6273e 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservation.java @@ -16,34 +16,32 @@ package org.springframework.transaction.support; -import io.micrometer.observation.docs.DocumentedObservation; -import io.micrometer.observation.docs.TagKey; +import io.micrometer.common.docs.KeyName; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationConvention; +import io.micrometer.observation.docs.ObservationDocumentation; -enum TransactionObservation implements DocumentedObservation { +enum TransactionObservation implements ObservationDocumentation { /** * Observation created when there was no previous transaction. If there was one, we will * continue it unless propagation is required. */ TX_OBSERVATION { - @Override - public String getName() { - return "spring.tx"; - } @Override - public String getContextualName() { - return "tx"; + public Class> getDefaultConvention() { + return DefaultTransactionObservationConvention.class; } @Override - public TagKey[] getLowCardinalityTagKeys() { - return LowCardinalityTags.values(); + public KeyName[] getLowCardinalityKeyNames() { + return LowCardinalityKeyNames.values(); } @Override - public TagKey[] getHighCardinalityTagKeys() { - return HighCardinalityTags.values(); + public KeyName[] getHighCardinalityKeyNames() { + return HighCardinalityKeyNames.values(); } @Override @@ -52,14 +50,14 @@ public String getPrefix() { } }; - enum LowCardinalityTags implements TagKey { + enum LowCardinalityKeyNames implements KeyName { /** * Name of the TransactionManager. */ TRANSACTION_MANAGER { @Override - public String getKey() { + public String asString() { return "spring.tx.transaction-manager"; } }, @@ -69,7 +67,7 @@ public String getKey() { */ READ_ONLY { @Override - public String getKey() { + public String asString() { return "spring.tx.read-only"; } }, @@ -79,7 +77,7 @@ public String getKey() { */ PROPAGATION_LEVEL { @Override - public String getKey() { + public String asString() { return "spring.tx.propagation-level"; } }, @@ -89,7 +87,7 @@ public String getKey() { */ ISOLATION_LEVEL { @Override - public String getKey() { + public String asString() { return "spring.tx.isolation-level"; } }, @@ -99,21 +97,21 @@ public String getKey() { */ TIMEOUT { @Override - public String getKey() { + public String asString() { return "spring.tx.timeout"; } } } - enum HighCardinalityTags implements TagKey { + enum HighCardinalityKeyNames implements KeyName { /** * Transaction name. */ NAME { @Override - public String getKey() { + public String asString() { return "spring.tx.name"; } } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationConvention.java similarity index 76% rename from spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java rename to spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationConvention.java index ed5a6d4ffb71..040b3cdfd805 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTagsProvider.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionObservationConvention.java @@ -16,14 +16,14 @@ package org.springframework.transaction.support; -import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationConvention; /** - * A {@link Observation.TagsProvider} for Spring Transaction. + * A {@link ObservationConvention} for Spring Transaction. * * @author Marcin Grzejszczak * @since 6.0.0 */ -public interface TransactionTagsProvider extends Observation.TagsProvider { +public interface TransactionObservationConvention extends ObservationConvention { } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java index d27e5eb30a95..a78dcab93340 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionTemplate.java @@ -18,7 +18,6 @@ import java.lang.reflect.UndeclaredThrowableException; -import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationRegistry; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -65,7 +64,7 @@ */ @SuppressWarnings("serial") public class TransactionTemplate extends DefaultTransactionDefinition - implements TransactionOperations, InitializingBean, Observation.TagsProviderAware { + implements TransactionOperations, InitializingBean { /** Logger available to subclasses. */ protected final Log logger = LogFactory.getLog(getClass()); @@ -75,7 +74,7 @@ public class TransactionTemplate extends DefaultTransactionDefinition private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; - private TransactionTagsProvider tagsProvider = new DefaultTransactionTagsProvider(); + private TransactionObservationConvention observationConvention = new DefaultTransactionObservationConvention(); /** * Construct a new TransactionTemplate for bean usage. @@ -126,9 +125,8 @@ public void setObservationRegistry(ObservationRegistry observationRegistry) { this.observationRegistry = observationRegistry; } - @Override - public void setTagsProvider(TransactionTagsProvider tagsProvider) { - this.tagsProvider = tagsProvider; + public void setObservationConvention(TransactionObservationConvention observationConvention) { + this.observationConvention = observationConvention; } @Override @@ -149,9 +147,9 @@ public T execute(TransactionCallback action) throws TransactionException } else { PlatformTransactionManager manager = this.transactionManager; - if (!this.observationRegistry.isNoOp()) { + if (!this.observationRegistry.isNoop()) { TransactionObservationContext context = new TransactionObservationContext(this, this.transactionManager); - manager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.tagsProvider); + manager = new ObservationPlatformTransactionManager(this.transactionManager, this.observationRegistry, context, this.observationConvention); } TransactionStatus status = manager.getTransaction(this); T result; diff --git a/spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionTagsProviderTests.java b/spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionObservationConventionTests.java similarity index 73% rename from spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionTagsProviderTests.java rename to spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionObservationConventionTests.java index a10cad108416..62340e69b034 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionTagsProviderTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/support/DefaultTransactionObservationConventionTests.java @@ -16,7 +16,7 @@ package org.springframework.transaction.support; -import io.micrometer.observation.Tags; +import io.micrometer.common.KeyValues; import org.junit.jupiter.api.Test; import org.springframework.transaction.TransactionManager; @@ -24,7 +24,7 @@ import static org.assertj.core.api.BDDAssertions.then; -class DefaultTransactionTagsProviderTests { +class DefaultTransactionObservationConventionTests { @Test void noTagsForContinuedTransaction() { @@ -34,8 +34,8 @@ void noTagsForContinuedTransaction() { }); context.setTransactionStatus(transactionStatus); - then(new DefaultTransactionTagsProvider().getLowCardinalityTags(context)).isSameAs(Tags.empty()); - then(new DefaultTransactionTagsProvider().getHighCardinalityTags(context)).isSameAs(Tags.empty()); + then(new DefaultTransactionObservationConvention().getLowCardinalityKeyValues(context)).isSameAs(KeyValues.empty()); + then(new DefaultTransactionObservationConvention().getHighCardinalityKeyValues(context)).isSameAs(KeyValues.empty()); } @Test @@ -48,7 +48,7 @@ void tagsForNewTransaction() { }); context.setTransactionStatus(transactionStatus); - then(new DefaultTransactionTagsProvider().getLowCardinalityTags(context)).isNotSameAs(Tags.empty()); - then(new DefaultTransactionTagsProvider().getHighCardinalityTags(context)).isNotSameAs(Tags.empty()); + then(new DefaultTransactionObservationConvention().getLowCardinalityKeyValues(context)).isNotSameAs(KeyValues.empty()); + then(new DefaultTransactionObservationConvention().getHighCardinalityKeyValues(context)).isNotSameAs(KeyValues.empty()); } } diff --git a/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java b/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java index 380ea1555d80..1f944aaf18db 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/support/ObservationPlatformTransactionManagerTests.java @@ -16,9 +16,9 @@ package org.springframework.transaction.support; -import io.micrometer.core.tck.TestObservationRegistry; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationRegistry; +import io.micrometer.observation.tck.TestObservationRegistry; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; @@ -26,7 +26,7 @@ import org.springframework.transaction.TransactionManager; import org.springframework.transaction.TransactionStatus; -import static io.micrometer.core.tck.TestObservationRegistryAssert.assertThat; +import static io.micrometer.observation.tck.TestObservationRegistryAssert.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.then; @@ -141,22 +141,22 @@ void rollbackForNoOpRegistryDelegatesToManagerWithoutCallingObservation() { private ObservationPlatformTransactionManager observationPlatformTransactionManager() { TransactionObservationContext context = new TransactionObservationContext(transactionDefinition(), transactionManager); given(this.delegate.getTransaction(any())).willReturn(new SimpleTransactionStatus()); - return new ObservationPlatformTransactionManager(this.delegate, this.observationRegistry, context, new DefaultTransactionTagsProvider()); + return new ObservationPlatformTransactionManager(this.delegate, this.observationRegistry, context, new DefaultTransactionObservationConvention()); } private ObservationPlatformTransactionManager noOpDelegateObservationPlatformTransactionManager(TransactionObservationContext context) { - return new ObservationPlatformTransactionManager(this.delegate, ObservationRegistry.NOOP, context, new DefaultTransactionTagsProvider()); + return new ObservationPlatformTransactionManager(this.delegate, ObservationRegistry.NOOP, context, new DefaultTransactionObservationConvention()); } private void assertThatRegistryHasAStoppedTxObservation(Observation.Scope scope) { assertThat(this.observationRegistry) .hasRemainingCurrentObservationSameAs(scope.getCurrentObservation()) .hasObservationWithNameEqualTo("spring.tx").that() - .hasLowCardinalityTag("spring.tx.isolation-level", "-1") - .hasLowCardinalityTag("spring.tx.propagation-level", "REQUIRED") - .hasLowCardinalityTag("spring.tx.read-only", "false") - .hasLowCardinalityTag("spring.tx.transaction-manager", "org.springframework.transaction.support.ObservationPlatformTransactionManagerTests$1") - .hasHighCardinalityTag("spring.tx.name", "foo") + .hasLowCardinalityKeyValue("spring.tx.isolation-level", "-1") + .hasLowCardinalityKeyValue("spring.tx.propagation-level", "REQUIRED") + .hasLowCardinalityKeyValue("spring.tx.read-only", "false") + .hasLowCardinalityKeyValue("spring.tx.transaction-manager", "org.springframework.transaction.support.ObservationPlatformTransactionManagerTests$1") + .hasHighCardinalityKeyValue("spring.tx.name", "foo") .hasBeenStarted() .hasBeenStopped(); }