astefanutti / metrics-aspectj

AspectJ integration for Dropwizard Metrics
Apache License 2.0
81 stars 39 forks source link

EL Expression is not working with @Metrics annotation #20

Open psehrawa opened 7 years ago

psehrawa commented 7 years ago

In gradle example Instead of using registry name I chose to use EL Expression ${this.registry} and program started throwing exceptions. Below is the code:

`package com;

import java.util.Random; import java.util.concurrent.TimeUnit;

import com.codahale.metrics.SharedMetricRegistries;

import com.codahale.metrics.ConsoleReporter; import com.codahale.metrics.MetricRegistry; import com.codahale.metrics.Meter; import com.codahale.metrics.Timer; import com.codahale.metrics.annotation.Timed;

import javax.el.ELProcessor;

import io.astefanutti.metrics.aspectj.Metrics;

// ~minimal App using metrics-aspectj public class App {

//public static final MetricRegistry registry = SharedMetricRegistries.getOrCreate("reg");

public static void main(String[] args) throws InterruptedException {
    //MetricsRegistry registry = SharedMetricRegistries.getOrCreate("reg");
    //App.class.getClassLoader().loadClass("javax.el.ELProcessor");
    ITimedMethod timedMethod = new TimedMethod();

    for (int i = 0; i < 10; ++i) {
        timedMethod.randomTimeTask();
    }

    // await console reporter
    Thread.sleep(1010);
}

}

// this interface is not strictly necessary, but was added to make the toggling // of explicit profiling easier. // // the class TimedMethodImplicit matches the ease of implementation for metrics // that is documented and made possible by metrics-aspectj. interface ITimedMethod { public void randomTimeTask() throws InterruptedException; }

class TimedMethod implements ITimedMethod { // to use the same metric registry when explicitly (read: cross-cuttingly) // profiling the App as well as when implicitly (read: aspect-oriented-ly) // profiling the App, here for the explicit case, get or create // (read: ~singleton) the metric registry using the same name as is used for // the implicit case. FYI the default name for the implicit case is // "metrics-registry". private static final MetricRegistry metrics = SharedMetricRegistries.getOrCreate("reg");

private ITimedMethod tmi;
private ITimedMethod tme;

public TimedMethod() {
    metricConsoleReporterStart();
    this.tmi = new TimedMethodImplicit(this.metrics);
    if (!AppConfig.isExplicitlyMeasuring) {
        this.tme = new TimedMethodNop();
    } else {
        this.tme = new TimedMethodExplicit(this.metrics);
    }
}

public void randomTimeTask() throws InterruptedException {
    tmi.randomTimeTask();
    tme.randomTimeTask();
}

private void metricConsoleReporterStart() {
    ConsoleReporter.forRegistry(this.metrics)
        .convertRatesTo(TimeUnit.SECONDS)
        .convertDurationsTo(TimeUnit.MILLISECONDS)
        .build()
        .start(1, TimeUnit.SECONDS);
}

}

@Metrics(registry = "${ this.registry }") class TimedMethodImplicit implements ITimedMethod { private final Random rand = new Random(); private final MetricRegistry registry;

public TimedMethodImplicit(MetricRegistry reg) {
    this.registry = reg;
}

public MetricRegistry getRegistry(){return this.registry;}

@Timed(name = "randomTimeTask")
public void randomTimeTask() throws InterruptedException {
    Thread.sleep(rand.nextInt(100));
}

} @Metrics(registry = "reg") class TimedMethodNop implements ITimedMethod { @Timed public void randomTimeTask() throws InterruptedException { } }

class TimedMethodExplicit implements ITimedMethod { private final MetricRegistry metrics; private final Timer timer;

private final Random rand = new Random();

public TimedMethodExplicit(MetricRegistry metrics) {
    this.metrics = metrics;
    this.timer = metrics.timer("request_times");
}

@Timed(name = "randomTimeTask")
public void randomTimeTask() throws InterruptedException {
    Timer.Context context = null;
    try {
        context = this.timer.time();
        Thread.sleep(rand.nextInt(100));
    } finally {
        if (context != null) {
            context.close();
        }
    }
}

} `

astefanutti commented 7 years ago

What exception do you have?

Note that you need to add an EL implementation as dependency, e.g., org.glassfish:javax.el for it to work.

There is a unit test that cover that use case so I would expect it to work: https://github.com/astefanutti/metrics-aspectj/blob/ec21a065e5accf7a34489a3ec5dcc10ebcd4689b/envs/el/src/test/java/io/astefanutti/metrics/aspectj/el/TimedMethodWithRegistryFromBeanPropertyTest.java.

psehrawa commented 7 years ago

Thanks for a prompt reply!

Here is the exception I get when I do gradle run

Exception in thread "main" javax.el.PropertyNotFoundException: The class 'com.TimedMethodImplicit' does not have a readable property 'registry'. at javax.el.BeanELResolver.getValue(BeanELResolver.java:354) at javax.el.CompositeELResolver.getValue(CompositeELResolver.java:188) at com.sun.el.parser.AstValue.getValue(AstValue.java:140) at com.sun.el.parser.AstValue.getValue(AstValue.java:204) at com.sun.el.ValueExpressionImpl.getValue(ValueExpressionImpl.java:226) at javax.el.ELProcessor.getValue(ELProcessor.java:129) at javax.el.ELProcessor.eval(ELProcessor.java:115) at io.astefanutti.metrics.aspectj.JavaxElMetricStrategy.resolveMetricRegistry(JavaxElMetricStrategy.java:44) at io.astefanutti.metrics.aspectj.MetricAspect$MetricAspect$4.metric(MetricAspect.aj:95) at io.astefanutti.metrics.aspectj.MetricAspect$MetricAspect$4.metric(MetricAspect.aj:1) at io.astefanutti.metrics.aspectj.AbstractMetricAspect.metricAnnotation(AbstractMetricAspect.aj:37) at io.astefanutti.metrics.aspectj.MetricAspect.ajc$after$io_astefanutti_metrics_aspectj_MetricAspect$1$c735687d(MetricAspect.aj:91) at com.TimedMethodImplicit.<init>(App.java:90) at com.TimedMethod.<init>(App.java:61) at com.App.main(App.java:26)

astefanutti commented 7 years ago

You need to have a getter method declared in your class for it to be a valid bean property, see:

https://github.com/astefanutti/metrics-aspectj/blob/ec21a065e5accf7a34489a3ec5dcc10ebcd4689b/envs/el/src/main/java/io/astefanutti/metrics/aspectj/el/TimedMethodWithRegistryFromBeanProperty.java#L31

psehrawa commented 7 years ago

I have a public getRegistry getter method. But still it throws this exception.

`@metrics(registry = "${ this.registry }") class TimedMethodImplicit implements ITimedMethod { private final Random rand = new Random(); private final MetricRegistry registry;

public TimedMethodImplicit(MetricRegistry reg) { this.registry = reg; }

public MetricRegistry getRegistry(){return this.registry;}

@Timed(name = "randomTimeTask") public void randomTimeTask() throws InterruptedException { Thread.sleep(rand.nextInt(100)); } }`

astefanutti commented 7 years ago

Could you try with the TimedMethodImplicit class being declared public?

astefanutti commented 7 years ago

Have you been able to work this out?

psehrawa commented 6 years ago

No its not working