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.

Sign up for GitHub

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

Jump to bottom

#309 Enable the average report to display more than one average #320

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

#309 Enable the average report to display more than one average #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f0f2c5
#309 Implemented functionality.
Jan 27, 2023
af325ed
#309 adjustementes for more user friendly handling.
Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions config/reportgenerator.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,35 @@ com.xceptance.xlt.reportgenerator.charts.height = 300
## The percentage of values taken when calculating the moving average series.
com.xceptance.xlt.reportgenerator.charts.movingAverage.percentageOfValues = 5

###############################################################################
#
# Additional Moving Average Settings
#
Comment on lines 86 to +92
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having two different formats to configure moving averages (one for the required moving average and one for the optional additional ones), it might be worth to think about how we can make a transition from the "old" to the "new" format.

This way, users don't have to riddle about how they should configure their average(s) but simply use the new format and add additional ones if desired. Of course, this would require some kind of compatibility for report-generator configurations created prior to XLT v7.

What about the following:

  • configuration of any moving average is done via properties com.xceptance.xlt.reportgenerator.charts.movingAverages.<index>.<percentage|time|requestCount>
  • drop property com.xceptance.xlt.reportgenerator.charts.movingAverage.percentageOfValues from this file
  • for compatibility with pre-XLT v7 report-generator configurations, still read value of property com.xceptance.xlt.reportgenerator.charts.movingAverage.percentageOfValues and if present, transform it internally into the new format (as if explicitly configured as com.xceptance.xlt.reportgenerator.charts.movingAverages.0.percentage = 5)
  • check if configuration makes use of both formats and error out in that case (w/ a proper error message saying that either the "old" format or the "new" format should be used)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a talk w/ the PO about that topic and he was fine w/ the current approach as the "default" moving average is used for all charts whereas the additional moving averages are used on the Averages Charts only.

Of course, consolidation of the two formats might still be an option (although some way to "mark" a moving average as default would be needed then).

@jowerner What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here for more input regarding this topic.

# When enabled this will draw additional moving averages in the same average chart.
# The values must be indexed and need a certain format.
#
# Format:
#
# com.xceptance.xlt.reportgenerator.charts.movingAverage.additional.<index>.<percentile|time|requests>
#
# index ........ Numeric value for indexing.
#
# percentile ... The percentage of values taken when calculating the moving average series.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a percentage of a value set is used, we should name it percentage.

#
# time ......... Time value to create the moving average. If the time is larger than the actual runtime,
# the value will not be shown in the charts.
# Allowed values are for example:
# example: 30m
# example: 1h30m
# example: 1h15m30s
#
# requests ..... Integer value for requests. If the amount is above the actual request value
# the average will not be shown in the charts.
Comment on lines +104 to +112
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description/explanation reads as if the configured value acts as some kind of minimum value that must be satisfied by a request so it is selected for the moving average.

According to the description of the feature request, the supported moving averages should select the "last X", "last X seconds" and "last X %" values, respectively.

Thus, I'm wondering whether the description should read as follows:

#  percentage . Amount of most recent values (percentage) to consider for calculating the moving average - last X% of all values.  
#  time ....... Time range of the sliding window used to calculate the moving average - requests within the configured time.
#  requests ... Number of most recent requests to consider for calculating the moving average - last X requests.

#
# example: com.xceptance.xlt.reportgenerator.charts.movingAverage.additional.1.percentile = 22
#
###############################################################################

## The percentiles to show in runtime data tables. Specify them as a comma-
## separated list of double values in the range (0, 100].
## Defaults to "50, 95, 99, 99.9". If left empty, no percentiles will be shown.
Expand Down
11 changes: 11 additions & 0 deletions src/main/java/com/xceptance/xlt/api/report/ReportProviderConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@
package com.xceptance.xlt.api.report;

import java.io.File;
import java.util.List;
import java.util.Properties;

import com.xceptance.xlt.report.util.MovingArerage;

/**
* The {@link ReportProviderConfiguration} interface provides access to general report generator settings as well as to
* report provider specific properties, which are both stored in the global configuration file.
Expand Down Expand Up @@ -87,6 +90,14 @@ public interface ReportProviderConfiguration
* @return the percentage
*/
public int getMovingAveragePercentage();

/**
* Returns a list of additional moving averages see {@link MovingArerage}. For example,
* with 5 percent and 1000 values, the moving average is generated from the last 50 values.
*
* @return additional moving averages
*/
public List<MovingArerage> getAdditonalMovingAverages();

/**
* Returns all the settings from the file "xlt/config/reportgenerator.properties" as raw properties. Use these
Expand Down
82 changes: 77 additions & 5 deletions src/main/java/com/xceptance/xlt/report/ReportGeneratorConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.io.IOException;
import java.lang.management.ManagementFactory;
import java.net.URI;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
Expand All @@ -35,6 +36,7 @@
import org.apache.commons.vfs2.FileObject;

import com.xceptance.common.util.AbstractConfiguration;
import com.xceptance.common.util.ParseUtils;
import com.xceptance.common.util.RegExUtils;
import com.xceptance.xlt.api.engine.Data;
import com.xceptance.xlt.api.report.ReportProvider;
Expand All @@ -51,6 +53,8 @@
import com.xceptance.xlt.report.mergerules.RequestProcessingRule;
import com.xceptance.xlt.report.providers.RequestTableColorization;
import com.xceptance.xlt.report.providers.RequestTableColorization.ColorizationRule;
import com.xceptance.xlt.report.util.MovingArerage;
import com.xceptance.xlt.report.util.MovingArerage.MovingAverages;

/**
* The ReportGeneratorConfiguration is the central place where all configuration information for the report generator
Expand Down Expand Up @@ -117,7 +121,7 @@ public enum ChartCappingMode
*/
public double parameter;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't add superfluous white-space (it come in handy to configure your IDE to show white-space characters).

private static final String PROP_PREFIX = XltConstants.XLT_PACKAGE_PATH + ".reportgenerator.";

private static final String PROP_RUNTIME_PERCENTILES = PROP_PREFIX + "runtimePercentiles";
Expand All @@ -139,6 +143,10 @@ public enum ChartCappingMode
private static final String PROP_SUFFIX_PERCENTILE = "percentile";

private static final String PROP_SUFFIX_SEGMENTATION = "segmentation";

private static final String PROP_SUFFIX_TIME = "time";

private static final String PROP_SUFFIX_REQUEST = "requests";

private static final String PROP_SUFFIX_ID = "id";

Expand All @@ -148,7 +156,11 @@ public enum ChartCappingMode

private static final String PROP_CHARTS_HEIGHT = PROP_CHARTS_PREFIX + "height";

private static final String PROP_CHARTS_MOV_AVG_PERCENTAGE = PROP_CHARTS_PREFIX + "movingAverage.percentageOfValues";
private static final String PROP_CHARTS_MOV_AVG = PROP_CHARTS_PREFIX + "movingAverage.";

private static final String PROP_CHARTS_MOV_AVG_ADD = PROP_CHARTS_MOV_AVG + "additional.";

private static final String PROP_CHARTS_MOV_AVG_PERCENTAGE = PROP_CHARTS_MOV_AVG + "percentageOfValues";

private static final String PROP_CHARTS_WIDTH = PROP_CHARTS_PREFIX + "width";

Expand Down Expand Up @@ -196,8 +208,10 @@ public enum ChartCappingMode

private final File homeDirectory;

private final int movingAveragePoints;

private int movingAveragePoints;

private List<MovingArerage> movingAverageEntries;
Comment on lines +211 to +213
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be declared as final as both fields do not change anymore once initialized.


private final List<String> outputFileNames;

private final List<Class<? extends ReportProvider>> reportProviderClasses;
Expand Down Expand Up @@ -365,6 +379,7 @@ public ReportGeneratorConfiguration(Properties xltProperties, final File overrid
{
testReportsRootDir = new File(homeDirectory, testReportsRootDir.getPath());
}
this.movingAveragePoints = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialization of this field is done in line 426. Please remove.

testReportsRootDirectory = testReportsRootDir;

File testResultsRootDir = getFileProperty(PROP_RESULTS_ROOT_DIR, new File(homeDirectory, XltConstants.RESULT_ROOT_DIR));
Expand Down Expand Up @@ -406,8 +421,11 @@ public ReportGeneratorConfiguration(Properties xltProperties, final File overrid
chartsCompressionLevel = getIntProperty(PROP_CHARTS_COMPRESSION_LEVEL, 6);
chartsWidth = getIntProperty(PROP_CHARTS_WIDTH, 600);
chartsHeight = getIntProperty(PROP_CHARTS_HEIGHT, 300);

// set the moving average and other averages, depending on set properties
movingAveragePoints = getIntProperty(PROP_CHARTS_MOV_AVG_PERCENTAGE, 5);

readAdditionalMovingAverages();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method should return the parsed moving averages such that field can be initialized properly.


threadCount = getIntProperty(PROP_THREAD_COUNT, ManagementFactory.getOperatingSystemMXBean().getAvailableProcessors());

removeIndexesFromRequestNames = getBooleanProperty(PROP_REMOVE_INDEXES_FROM_REQUEST_NAMES, false);
Expand Down Expand Up @@ -674,6 +692,12 @@ public int getMovingAveragePercentage()
{
return movingAveragePoints;
}

@Override
public List<MovingArerage> getAdditonalMovingAverages()
{
return movingAverageEntries;
}

public List<String> getOutputFileNames()
{
Expand Down Expand Up @@ -1539,4 +1563,52 @@ private Pattern compileRegEx(final String regEx, final String propertyName)
propertyName, ex.getMessage()));
}
}

private void readAdditionalMovingAverages()
{
movingAverageEntries = new ArrayList<>();
final Set<Integer> additionalMovingAveragesNumbers = new TreeSet<>();
Set<String> propertyKeysWithPrefix = getPropertyKeyFragment(PROP_CHARTS_MOV_AVG_ADD);

for (final String s : propertyKeysWithPrefix)
{
checkForLeadingZeros(s);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention of this method was to report request-merge-rule configuration issues. In case it should also check for moving-average configuration issues, it has to be adapted accordingly.

additionalMovingAveragesNumbers.add(Integer.parseInt(s));
}

for (final int i : additionalMovingAveragesNumbers)
{
final String basePropertyName = PROP_CHARTS_MOV_AVG_ADD;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Value might be changed to PROP_CHARTS_MOV_AVG_ADD + i + "." (or at least PROP_CHARTS_MOV_AVG_ADD + i) such that string concatenation for the common property prefix is done only once.


String timeValue = getStringProperty(basePropertyName + i + "." + PROP_SUFFIX_TIME, "");
int percentageValue = getIntProperty(basePropertyName + i + "." + PROP_SUFFIX_PERCENTILE, 0);
int requestValue = getIntProperty(basePropertyName + i + "." + PROP_SUFFIX_REQUEST, 0);
Comment on lines +1583 to +1585
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need to use defaults that allow to check whether the property was defined at all so that you can check whether the user has specified all three, two of the three or none of the three possible values for the given index.

Suggested change
String timeValue = getStringProperty(basePropertyName + i + "." + PROP_SUFFIX_TIME, "");
int percentageValue = getIntProperty(basePropertyName + i + "." + PROP_SUFFIX_PERCENTILE, 0);
int requestValue = getIntProperty(basePropertyName + i + "." + PROP_SUFFIX_REQUEST, 0);
String timeValue = getStringProperty(basePropertyName + i + "." + PROP_SUFFIX_TIME, null);
int percentageValue = getIntProperty(basePropertyName + i + "." + PROP_SUFFIX_PERCENTILE, -1);
int requestValue = getIntProperty(basePropertyName + i + "." + PROP_SUFFIX_REQUEST, -1);


if (StringUtils.isNotBlank(timeValue))
{
try
{
movingAverageEntries.add(new MovingArerage(MovingAverages.TIME_TO_USE, ParseUtils.parseTimePeriod(timeValue) * 1000L, timeValue));
}
catch (ParseException e)
{
throw new XltException(String.format("The value '%s' of property '%s' is not a valid time value.", basePropertyName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is misleading; the variable basePropertyName does not contain the value of the appropriate property and the property is not PROP_SUFFIX_TIME but basePropertyName + i + "." + PROP_SUFFIX_TIME.

PROP_SUFFIX_TIME));
}
}
else if (percentageValue > 0)
{
movingAverageEntries.add(new MovingArerage(MovingAverages.PERCENTAGE_OF_VALUES, percentageValue));
}
else if (requestValue > 0)
{
movingAverageEntries.add(new MovingArerage(MovingAverages.AMOUNT_OF_REQUESTS, requestValue));
}
else
{
throw new XltException(String.format("The keyword '%s' of property is not a valid keyword, allowed keywords are '%s', '%s', '%s'.", basePropertyName + i,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is misleading; PROP_CHARTS_MOV_AVG_ADD + i is no keyword but simply a prefix of a property.

What about to change it to something like (simplified version may apply)

String.format("Must specify exactly one of the properties '%s', '%s' or '%s' to configure moving average with index '%d'",  PROP_CHARTS_MOV_AVG_ADD + i +"." + PROP_SUFFIX_TIME , PROP_CHARTS_MOV_AVG_ADD + i +"." + PROP_SUFFIX_PERCENTILE, PROP_CHARTS_MOV_AVG_ADD + i +"." + PROP_SUFFIX_REQUEST, i)

PROP_SUFFIX_PERCENTILE, PROP_SUFFIX_TIME, PROP_SUFFIX_REQUEST));
}
}
}
}
25 changes: 25 additions & 0 deletions src/main/java/com/xceptance/xlt/report/providers/AbstractDataProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@
package com.xceptance.xlt.report.providers;

import java.io.File;
import java.util.List;

import com.xceptance.xlt.api.engine.Data;
import com.xceptance.xlt.api.report.AbstractReportProvider;
import com.xceptance.xlt.api.report.ReportProviderConfiguration;
import com.xceptance.xlt.report.ReportGeneratorConfiguration.ChartCappingInfo;
import com.xceptance.xlt.report.util.MovingArerage;

/**
* The {@link AbstractDataProcessor} class provides common functionality of a typical data processor. A data processor
Expand All @@ -37,6 +39,8 @@ public abstract class AbstractDataProcessor
private File csvDir;

private int movingAveragePercentage;

private List<MovingArerage> additionalMovingAverages;

private ChartCappingInfo chartCappingInfo;

Expand Down Expand Up @@ -72,6 +76,7 @@ protected AbstractDataProcessor(final String name, final AbstractReportProvider
setChartHeight(config.getChartHeight());

setMovingAveragePercentage(config.getMovingAveragePercentage());
setAdditionalMovingAverages(config.getAdditonalMovingAverages());
}

/**
Expand Down Expand Up @@ -144,6 +149,16 @@ public int getMovingAveragePercentage()
{
return movingAveragePercentage;
}

/**
* Returns the list of all additional moving averages.
*
* @return additional moving averages
*/
public List<MovingArerage> getAdditonalMovingAverages()
{
return additionalMovingAverages;
}

/**
* Returns the timer name.
Expand Down Expand Up @@ -253,6 +268,16 @@ public void setMovingAveragePercentage(final int movingAveragePercentage)
this.movingAveragePercentage = movingAveragePercentage;
}

/**
* Sets the values for additional moving averages.
*
* @param additionalMovingAverages all additional moving averages
*/
public void setAdditionalMovingAverages(final List<MovingArerage> additionalMovingAverages)
{
this.additionalMovingAverages = additionalMovingAverages;
}

/**
* Sets the chart capping info that describes how to cap a run time chart.
*
Expand Down
Loading