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

Fixed the EWMA to work regardless of the TICK_THREAD interval settings in #6

Closed
wants to merge 1 commit into from

Conversation

jebl01
Copy link
Contributor

@jebl01 jebl01 commented Apr 12, 2011

Fixed the EWMA to work regardless of the TICK_THREAD interval settings in e.g. MeterMetric.
Before this fix, an EWMA was always created with a default interval of 5 seconds which gave incorrect
results when createing a MeterMetric with different values (for interval and intervalUnit).

The following simple test now gives (aproximaapproximately) the same output:

MeterMetric meter = MeterMetric.newMeter(1, TimeUnit.SECONDS, "test event", TimeUnit.SECONDS);

for (int i = 0; i < 100; i++)
{
Thread.sleep(100);
meter.mark();
}

System.out.println(meter.meanRate());
System.out.println(meter.oneMinuteRate());

…s in e.g. MeterMetric.

Before this fix, an EWMA was always created with a default interval of 5 seconds which gave incorrect
results when createing a MeterMetric with different values (for interval and intervalUnit).

The following simple test now gives (aproximaapproximately) the same output:

MeterMetric meter = MeterMetric.newMeter(1, TimeUnit.SECONDS, "test event", TimeUnit.SECONDS);

for (int i = 0; i < 100; i++)
{
	Thread.sleep(100);
	meter.mark();
}

System.out.println(meter.meanRate());
System.out.println(meter.oneMinuteRate());
@codahale
Copy link
Contributor

Fixed in 37f88c1 instead, by removing the second factory method and disallowing arbitrary intervals.

@codahale codahale closed this Apr 15, 2011
@jebl01
Copy link
Contributor Author

jebl01 commented Apr 15, 2011

Ok, Thank you.
This pull request was actually only a preparation for an other planned request. I have added a req/interval counter alongside the others, which will give you a req/sec counter if you use an interval of one sec (req/sec is something we need to feed our "realtime" graphing monitors displayed on eight 40" LCD's).
I guess this has to be done some other way then, maybe by scheduling a separate job for this for each meter.

We are currently evaluating the Metrics lib in favor to our own (much similar). But to feel confident we need the per sec counter :-)

Great work btw!

/Jesper

On 15 apr 2011, at 05:59, codahalereply@reply.github.com wrote:

Fixed in 37f88c1 instead, by removing the second factory method and disallowing arbitrary intervals.

Reply to this email directly or view it on GitHub:
codahale/metrics#6 (comment)

@codahale
Copy link
Contributor

I think you may be misunderstanding what the interval is vs. what the rate unit is.

The interval is the interval between ticks for the exponentially weighted moving averages. This is no longer a public value because it's an implementation detail, not a meaningful external value.

The rate unit is the unit of time that events are averaged over (for example requests/second). If you want req/sec, create a meter with a rate unit of TimeUnit.SECONDS and mark it each time you process a request. The resulting mean, 1-minute, 5-minute, and 15-minute rates will all be in requests per second.

Coda Hale
http://codahale.com

On Thursday, April 14, 2011 at 10:40 PM, jebl01 wrote:

Ok, Thank you.
This pull request was actually only a preparation for an other planned request. I have added a req/interval counter alongside the others, which will give you a req/sec counter if you use an interval of one sec (req/sec is something we need to feed our "realtime" graphing monitors displayed on eight 40" LCD's).
I guess this has to be done some other way then, maybe by scheduling a separate job for this for each meter.

We are currently evaluating the Metrics lib in favor to our own (much similar). But to feel confident we need the per sec counter :-)

Great work btw!

/Jesper

On 15 apr 2011, at 05:59, codahalereply@reply.github.com wrote:

Fixed in 37f88c1 instead, by removing the second factory method and disallowing arbitrary intervals.

Reply to this email directly or view it on GitHub:
codahale/metrics#6 (comment)

Reply to this email directly or view it on GitHub:
codahale/metrics#6 (comment)

@jebl01
Copy link
Contributor Author

jebl01 commented Apr 15, 2011

Yes, but it will still be a mean value based on the last 1,5,15 minutes, not an actual snapshot of the latest one second, right!
That's why I needed the interval to be more than an implementation detail.
But as a said, a second scheduled job with a rate of one second can be used instead.

/Jesper

On 15 apr 2011, at 07:49, codahalereply@reply.github.com wrote:

I think you may be misunderstanding what the interval is vs. what the rate unit is.

The interval is the interval between ticks for the exponentially weighted moving averages. This is no longer a public value because it's an implementation detail, not a meaningful external value.

The rate unit is the unit of time that events are averaged over (for example requests/second). If you want req/sec, create a meter with a rate unit of TimeUnit.SECONDS and mark it each time you process a request. The resulting mean, 1-minute, 5-minute, and 15-minute rates will all be in requests per second.

Coda Hale
http://codahale.com

On Thursday, April 14, 2011 at 10:40 PM, jebl01 wrote:

Ok, Thank you.
This pull request was actually only a preparation for an other planned request. I have added a req/interval counter alongside the others, which will give you a req/sec counter if you use an interval of one sec (req/sec is something we need to feed our "realtime" graphing monitors displayed on eight 40" LCD's).
I guess this has to be done some other way then, maybe by scheduling a separate job for this for each meter.

We are currently evaluating the Metrics lib in favor to our own (much similar). But to feel confident we need the per sec counter :-)

Great work btw!

/Jesper

On 15 apr 2011, at 05:59, codahalereply@reply.github.com wrote:

Fixed in 37f88c1 instead, by removing the second factory method and disallowing arbitrary intervals.

Reply to this email directly or view it on GitHub:
codahale/metrics#6 (comment)

Reply to this email directly or view it on GitHub:
codahale/metrics#6 (comment)

Reply to this email directly or view it on GitHub:
codahale/metrics#6 (comment)

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.

None yet

2 participants