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

Adds a time boxed sampling for backend listeners #237

Closed
wants to merge 3 commits into
base: trunk
from

Conversation

Projects
None yet
5 participants
@max3163
Contributor

max3163 commented Dec 9, 2016

No description provided.

@FSchumacher

Thanks for the contribution.
Overall it looks good. Maybe you could also describe the use case for this feature compared to the fixed-size one.

/**
* Metrics are sent into boxes which can be {@link #FIXED a fixed-size sliding window} or {@link #TIMED time boxed}.
* @author Logan Mzz

This comment has been minimized.

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

We don't use the author doc tag.

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

We don't use the author doc tag.

stat.clear();
}
break;
default: throw new UnsupportedOperationException(windowMode.name());

This comment has been minimized.

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

Seems like a formatting error

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

Seems like a formatting error

// http://commons.apache.org/proper/commons-math/userguide/stat.html
break;
case TIMED:
for (DescriptiveStatistics stat : windowedStats) {

This comment has been minimized.

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

JMeter now uses java 8, so we could use the newer syntax :)

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

JMeter now uses java 8, so we could use the newer syntax :)

int slidingWindowSize = JMeterUtils.getPropDefault("backend_metrics_window", 100);
// Limit to sliding window of SLIDING_WINDOW_SIZE values
for (DescriptiveStatistics stat : stats) {

This comment has been minimized.

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

Formatting issue

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

Formatting issue

@@ -917,6 +917,8 @@ summariser.name=summary
# BackendListener - configuration
#---------------------------------------------------------------------------
#
# Backend metrics window mode (fixed=fixed-size window, timed=time boxed)
#backend_metrics_window_mode=fixed

This comment has been minimized.

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

Documentation is missing for this new feature. It should be mentioned in the properties section and the users manual.

@FSchumacher

FSchumacher Dec 11, 2016

Contributor

Documentation is missing for this new feature. It should be mentioned in the properties section and the users manual.

@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Dec 14, 2016

Contributor

Hello @max3163,
Thank you for your contribution.
Could you (as requested by Felix) give more details on the use case for this PR ?
Thank you
Regards

Contributor

pmouawad commented Dec 14, 2016

Hello @max3163,
Thank you for your contribution.
Could you (as requested by Felix) give more details on the use case for this PR ?
Thank you
Regards

@max3163

This comment has been minimized.

Show comment
Hide comment
@max3163

max3163 Dec 14, 2016

Contributor

When you use the fixed metric window mode, all samples metrics are saved in a sliding array with a fix size, you send each second to your backend a trend of this values
When you use the time boxed mode, all samples metrics are reset in order to send actual values for the last elapse time.
I you use it with a Influxdb backend (I'm thinking to send a PR to share it too) with a pooling of 5 seconds.

Contributor

max3163 commented Dec 14, 2016

When you use the fixed metric window mode, all samples metrics are saved in a sliding array with a fix size, you send each second to your backend a trend of this values
When you use the time boxed mode, all samples metrics are reset in order to send actual values for the last elapse time.
I you use it with a Influxdb backend (I'm thinking to send a PR to share it too) with a pooling of 5 seconds.

@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Dec 28, 2016

Contributor

Hello Team,
PR looks fine to me.
Do you agree with enhancement ?
Thank you

Contributor

pmouawad commented Dec 28, 2016

Hello Team,
PR looks fine to me.
Do you agree with enhancement ?
Thank you

@Wyatts

This comment has been minimized.

Show comment
Hide comment
@Wyatts

Wyatts Dec 28, 2016

This sounds good if it's what I think it is. Definitely would appreciate a clear description of what it precisely does, though.

Wyatts commented Dec 28, 2016

This sounds good if it's what I think it is. Definitely would appreciate a clear description of what it precisely does, though.

@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Dec 30, 2016

Contributor

@max3163 , are you using GraphiteBackendListener or something else (I don't fully understand your comment on "I'm thinking to send a PR to share it too".
If not, why aren't you using it ?
Thanks

Contributor

pmouawad commented Dec 30, 2016

@max3163 , are you using GraphiteBackendListener or something else (I don't fully understand your comment on "I'm thinking to send a PR to share it too".
If not, why aren't you using it ?
Thanks

@max3163

This comment has been minimized.

Show comment
Hide comment
@max3163

max3163 Dec 31, 2016

Contributor

@pmouawad
We use ur own BackendListener to send metric on InfluxDB with his HTTP API and follow his own shema design ( see : Influxdb data design ) which is more better than use the Graphite plugin.
If you want, we can share this BackendListener.

Contributor

max3163 commented Dec 31, 2016

@pmouawad
We use ur own BackendListener to send metric on InfluxDB with his HTTP API and follow his own shema design ( see : Influxdb data design ) which is more better than use the Graphite plugin.
If you want, we can share this BackendListener.

@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Jan 2, 2017

Contributor

Hello,
Thanks for proposal.
What is the quantity of code ? Does it have any dependency on Influx library or it is standalone using socket ?

Thank you

Contributor

pmouawad commented Jan 2, 2017

Hello,
Thanks for proposal.
What is the quantity of code ? Does it have any dependency on Influx library or it is standalone using socket ?

Thank you

@max3163

This comment has been minimized.

Show comment
Hide comment
@max3163

max3163 Jan 4, 2017

Contributor

Hi,

I just sent you a new PR ( #246 ) for the Influxdb backend.
It's about 650 lines of code ( test class included ) and with no dependency on Influxdb library.

Contributor

max3163 commented Jan 4, 2017

Hi,

I just sent you a new PR ( #246 ) for the Influxdb backend.
It's about 650 lines of code ( test class included ) and with no dependency on Influxdb library.

max3163 added a commit to max3163/jmeter that referenced this pull request Jan 4, 2017

Delete JUnit test
These tests only work with the #237 PR merged and timed mode box

@asfgit asfgit closed this in 881a5d1 Jan 15, 2017

@max3163 max3163 deleted the max3163:POW-319 branch Apr 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment