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

Adding basic metrics support #177

Closed
wants to merge 20 commits into from
Closed

Adding basic metrics support #177

wants to merge 20 commits into from

Conversation

rhart
Copy link
Member

@rhart rhart commented Oct 12, 2013

Initial commit for your review on the approach. I chose to use AOP as it means less noise in the code and will be more flexible going forward than using Guice method interceptors e.g. for non Guice objects. Can easily be changed though if the extra dependencies and/or JVM arg required to activate are a problem.

Usage would be as follows

ratpack {
    modules {
        register new MetricsModule()
        register new MetricsJmxReporterModule()
    }

    handlers { MetricRegistry metrics ->
        get {
          render groovyTemplate("index.html", title: "My Ratpack App")
        }

        prefix("foo") {
            get {
              metrics.meter("foo get handler").mark()
              response.send("foo")
            }
        }

        prefix("bar") {
            get {
                render groovyTemplate("index.html", title: "My Ratpack App")
            }
        }

        assets "public"
    }
}

You can see that custom metrics would be easily supported. Would also intend on providing an annotation to make it even easier.
Additional modules would be created to support the other reporters.

JVM arg is required though to active AOP

run {
    jvmArgs "-javaagent:${configurations.runtime.files.find {it.name == 'aspectjweaver-1.7.3.jar'}.path}"
    systemProperty "ratpack.reloadable", "true"
}

metrics

Feel free to tear into it and I will make any changes :)

@rhart
Copy link
Member Author

rhart commented Nov 23, 2013

I've made the changes from your comments but as usual this pull request is looking messy now. Do you want me to create a new one? Not sure what I'm doing wrong but will try and find out.

The main commit of interest is this one rhart@afd9c0e

It's really hard to hook into the close event stuff from the aspect. I just don't have access to the Context. So, I've created a new statefull handler in the Netty pipeline before the Ratpack one. I think this will give a more accurate response time actually. Hopefully you think this is the way to go too.

@ldaley
Copy link
Member

ldaley commented Nov 24, 2013

I think the commit history mess might be from rebasing on pull instead of merging.

I'm going to pull this and I'll clean it up on my side.

@ldaley
Copy link
Member

ldaley commented Nov 24, 2013

Pulled.

@ldaley ldaley closed this Nov 24, 2013
@rhart rhart deleted the metrics branch November 25, 2013 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants