Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add metrics support (Codahale) #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions btm/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@
<scope>provided</scope>
<optional>true</optional>
</dependency>
<dependency>
<groupId>com.codahale.metrics</groupId>
<artifactId>metrics-core</artifactId>
<version>${codahale.metrics.version}</version>
<scope>provided</scope>
<optional>true</optional>
</dependency>
</dependencies>

<build>
Expand Down
20 changes: 20 additions & 0 deletions btm/src/main/java/bitronix/tm/Configuration.java
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public class Configuration implements Service {
private volatile String resourceConfigurationFilename;
private volatile boolean conservativeJournaling;
private volatile String jdbcProxyFactoryClass;
private volatile String metricsFactoryClass;

protected Configuration() {
try {
Expand Down Expand Up @@ -125,6 +126,7 @@ protected Configuration() {
resourceConfigurationFilename = getString(properties, "bitronix.tm.resource.configuration", null);
conservativeJournaling = getBoolean(properties, "bitronix.tm.conservativeJournaling", false);
jdbcProxyFactoryClass = getString(properties, "bitronix.tm.jdbcProxyFactoryClass", "auto");
metricsFactoryClass = getString(properties, "bitronix.tm.metricsFactoryClass", "auto");
} catch (IOException ex) {
throw new InitializationException("error loading configuration", ex);
}
Expand Down Expand Up @@ -683,6 +685,24 @@ public void setJdbcProxyFactoryClass(String jdbcProxyFactoryClass) {
this.jdbcProxyFactoryClass = jdbcProxyFactoryClass;
}

/**
* Get the factory class for creating metrics instances.
* The default value is "auto", set it to "none" if you don't want metrics.
*
* @return the name of the factory class
*/
public String getMetricsFactoryClass() {
return metricsFactoryClass;
}

/**
* Set the name of the factory class for creating metrics instances.
*
* @param metricsFactoryClass the name of the metrics factory class
*/
public void setMetricsFactoryClass(String metricsFactoryClass) {
this.metricsFactoryClass = metricsFactoryClass;
}

/**
* {@link bitronix.tm.resource.ResourceLoader} configuration file name. {@link bitronix.tm.resource.ResourceLoader}
Expand Down
75 changes: 75 additions & 0 deletions btm/src/main/java/bitronix/tm/metric/CodahaleMetrics.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package bitronix.tm.metric;

import bitronix.tm.TransactionManagerServices;
import com.codahale.metrics.JmxReporter;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.Slf4jReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.concurrent.TimeUnit;

/**
* CodahaleMetrics - com.codahale.metrics based Metrics implementation.
* <p/>
* It may report the cummulated metrics to both the current log or platform JMX server.
*
* @author Vlad Mihalcea
*/
public class CodahaleMetrics implements Metrics {

private final static Logger log = LoggerFactory.getLogger(CodahaleMetrics.class);

private final String domain;

private final MetricRegistry metricRegistry;
private final Slf4jReporter logReporter;
private final JmxReporter jmxReporter;

public CodahaleMetrics(String domain) {
this.domain = domain;
this.metricRegistry = new MetricRegistry();
this.logReporter = Slf4jReporter
.forRegistry(metricRegistry)
.outputTo(log)
.build();
if (!TransactionManagerServices.getConfiguration().isDisableJmx()) {
jmxReporter = JmxReporter
.forRegistry(metricRegistry)
.inDomain(domain)
.build();
} else {
jmxReporter = null;
}
}

public CodahaleMetrics(Class<?> clazz, String uniqueName) {
this(MetricRegistry.name(clazz, uniqueName));
}

public String getDomain() {
return domain;
}

public void updateHistogram(String name, long value) {
metricRegistry.histogram(name).update(value);
}

public void updateTimer(String name, long value, TimeUnit timeUnit) {
metricRegistry.timer(name).update(value, timeUnit);
}

public void start() {
logReporter.start(5, TimeUnit.MINUTES);
if (jmxReporter != null) {
jmxReporter.start();
}
}

public void stop() {
logReporter.stop();
if (jmxReporter != null) {
jmxReporter.stop();
}
}
}
17 changes: 17 additions & 0 deletions btm/src/main/java/bitronix/tm/metric/CodahaleMetricsFactory.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package bitronix.tm.metric;

/**
* CodahaleMetricsFactory - com.codahale.metrics based MetricsFactory implementation.
*
* @author Vlad Mihalcea
*/
public class CodahaleMetricsFactory implements MetricsFactory {

public Metrics metrics(String domain) {
return new CodahaleMetrics(domain);
}

public Metrics metrics(Class<?> clazz, String uniqueName) {
return new CodahaleMetrics(clazz, uniqueName);
}
}
46 changes: 46 additions & 0 deletions btm/src/main/java/bitronix/tm/metric/Metrics.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package bitronix.tm.metric;

import java.io.Serializable;
import java.util.concurrent.TimeUnit;

/**
* Metrics - This interface defines a metrics basic behavior. This is required to isolate Bitronix from
* external Metrics dependencies.
*
* @author Vlad Mihalcea
*/
public interface Metrics extends Serializable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why extending Serializable here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be removed.


/**
* Each metrics must define the unique domain it represents.
*
* @return metrics domain
*/
String getDomain();

/**
* Update the given histogram
*
* @param name histogram name
* @param value histogram instant value
*/
void updateHistogram(String name, long value);

/**
* Update the given timer
*
* @param name timer name
* @param value timer instant value
*/
void updateTimer(String name, long value, TimeUnit timeUnit);

/**
* Start metrics reporting.
*/
void start();

/**
* Stop metrics reporting.
*/
void stop();
}
21 changes: 21 additions & 0 deletions btm/src/main/java/bitronix/tm/metric/MetricsAware.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package bitronix.tm.metric;

/**
* MetricsAware - Interface to obtaining the current associated Metrics
*
* @author Vlad Mihalcea
*/
public interface MetricsAware {

/**
* Initialize and bind a metrics to the current object.
*/
void initializeMetrics();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this method just 'initialize'? That would look better IMHO.


/**
* Get the current object metrics.
*
* @return current object metrics.
*/
Metrics getMetrics();
}
92 changes: 92 additions & 0 deletions btm/src/main/java/bitronix/tm/metric/MetricsFactory.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package bitronix.tm.metric;

import bitronix.tm.TransactionManagerServices;
import bitronix.tm.internal.BitronixRuntimeException;
import bitronix.tm.utils.ClassLoaderUtils;

/**
* MetricsFactory - Factory for Metrics instances.
* <p/>
* By default (the "auto" mode) it tries to locate the Codahale metrics registry, and therefore the Instance will
* reference a CodahaleMetricsFactory instance.
* <p/>
* If you choose the "none" mode, you won;t get any MetricsFactory.
* <p/>
* If you choose a custom factory class name, then you may use your own Metrics implementation.
*
* @author Vlad Mihalcea
*/
public interface MetricsFactory {

/**
* Instance resolving utility.
*/
public static class Instance {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like this instance holder. I think this code should be moved to the TransactionManagerServices class.


/* Classes should use MetricsFactory.INSTANCE to access the factory */
static MetricsFactory INSTANCE = Initializer.initialize(
TransactionManagerServices.getConfiguration().getMetricsFactoryClass());

/**
* Check if there is any initialized instance.
*
* @return is any instance available.
*/
public static boolean exists() {
return INSTANCE != null;
}

/**
* Get the initialized instance.
*
* @return the initialized instance.
*/
public static MetricsFactory get() {
return INSTANCE;
}
}

/**
* Get the domain related metrics.
*
* @param domain domain
* @return domain related metrics
*/
Metrics metrics(String domain);

/**
* Get a Class bound domain metrics, embedding an uniqueName.
*
* @param clazz class as the metrics domain context
* @param uniqueName unique name to ensure domain uniqueness
* @return domain related metrics
*/
Metrics metrics(Class<?> clazz, String uniqueName);

/**
* Initializer class used to initialize the factory.
*/
class Initializer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark as for the Instance class: this code should be moved to the TransactionManagerServices in a new method called getMetricsFactory() or something like that.

By the way, this code also isn't thread-safe, you should consider using Atomic Refs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should have been made final, and that would be thread-safe as in this example:
http://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom#Example_Java_Implementation

It has to be set during a test, so that's why I removed the final keyword.
When being moved to TransactionManagerServices a solution should be found for this issue as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By doing that you basically ignore the restart problem.

Have a look at:
https://github.com/bitronix/btm/blob/master/btm/src/main/java/bitronix/tm/TransactionManagerServices.java#L112
and
https://github.com/bitronix/btm/blob/master/btm/src/main/java/bitronix/tm/BitronixTransactionManager.java#L348

to get an example of how BTM initializes and cleans up its components. Not a design to be very proud of, but at least that's been kept consistent.

BTW, all those service interfaces extend bitronix.tm.utils.Service which you should also consider doing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it should be moved to the TransactionManagerServices, and probably remove the Metrics reference from the ResourceBean. It should get it from TransactionManagerServices, and fetching it on demand won't probably add too much overhead.

static MetricsFactory initialize(String metricFactoryClass) {
if ("none".equals(metricFactoryClass)) {
return null;
}
if ("auto".equals(metricFactoryClass)) {
try {
ClassLoaderUtils.loadClass("com.codahale.metrics.MetricRegistry");
return new CodahaleMetricsFactory();
} catch (ClassNotFoundException cnfe) {
//DO NOTHING
}
} else if (!metricFactoryClass.isEmpty()) {
try {
Class<?> clazz = ClassLoaderUtils.loadClass(metricFactoryClass);
return (MetricsFactory) clazz.newInstance();
} catch (Exception ex) {
throw new BitronixRuntimeException("error initializing MetricsFactory", ex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should throw InitializationException here.

}
}
return null;
}
}
}
27 changes: 26 additions & 1 deletion btm/src/main/java/bitronix/tm/resource/common/ResourceBean.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
*/
package bitronix.tm.resource.common;

import bitronix.tm.metric.Metrics;
import bitronix.tm.metric.MetricsAware;
import bitronix.tm.metric.MetricsFactory;
import bitronix.tm.utils.ManagementRegistrar;

import java.io.Serializable;
import java.util.Properties;

Expand All @@ -25,7 +30,7 @@
* @author Ludovic Orban
*/
@SuppressWarnings("serial")
public abstract class ResourceBean implements Serializable {
public abstract class ResourceBean implements Serializable, MetricsAware {

private volatile String className;
private volatile String uniqueName;
Expand All @@ -49,6 +54,8 @@ public abstract class ResourceBean implements Serializable {

private volatile transient int createdResourcesCounter;

private volatile transient Metrics metrics;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the bean gets serialized and then deserialized, this field will get nulled out and getMetrics() will return null. You should probably do something about that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A readResolve methods should be added to reinitialize the metrics reference (calling initializeMetrics()). If we move it to TransactionManagerServices we don;t even need it there.


/**
* Initialize all properties with their default values.
*/
Expand Down Expand Up @@ -362,4 +369,22 @@ public boolean isDisabled() {
public int incCreatedResourcesCounter() {
return this.createdResourcesCounter++;
}

/**
* Initialize a resource metrics, using the class and the unique name to build the metrics domain.
*/
public void initializeMetrics() {
if (metrics == null && MetricsFactory.Instance.exists()) {
metrics = MetricsFactory.Instance.get()
.metrics(getClass(), ManagementRegistrar.makeValidName(getUniqueName()));
}
}

/**
* Get the current associated metrics.
* @return current associated metrics.
*/
public Metrics getMetrics() {
return metrics;
}
}
Loading