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

Fix to 65446 - Move statistic_window property to user.properties file #671

Closed
wants to merge 1 commit into from

Conversation

rithvikp1998
Copy link
Contributor

Description

PercentileAggregator class reads the value for statistic_window using the following line:

private static final int SLIDING_WINDOW_SIZE = JMeterUtils.getPropDefault(
            ReportGeneratorConfiguration.REPORT_GENERATOR_KEY_PREFIX
                    + ReportGeneratorConfiguration.KEY_DELIMITER
                    + "statistic_window", 20000);

getPropDefault() calls appProperties.getProperty(). But in the appProperties we are only loading jmeter.properties, user.properties, system.properties etc and not reportgenerator.properties where the value for statistic_window is supposed to be set. So the default value of 20,000 is being picked irrespective of what is set in the properties file.

There are multiple ways to fix this issue but I chose this approach because it is the least intrusive.

Motivation and Context

https://bz.apache.org/bugzilla/show_bug.cgi?id=65446

How Has This Been Tested?

I built the code with the new changes and generated dashboards with some debug log statements in the code. The dashboards were getting generated correctly and the log statements are printing whatever property value I am setting. (Without this patch, they always print 20,000 as the size of the statistic window even when I am setting a different value in reportgenerator.properties file)

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.

@FSchumacher
Copy link
Contributor

I think, this is a valid fix, as the documentation does not state, where the property would have to be placed and reportgenerator.properties is not used for general properties and there are a few other reportgenerator related properties already defined in users.properties.

@rithvikp1998
Copy link
Contributor Author

@FSchumacher Can we get this merged then? Or is there anything else I need to do?

@asfgit asfgit closed this in 228ce43 Aug 21, 2021
@FSchumacher
Copy link
Contributor

Thanks for the PR it has been merged.

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