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

Glenmorangie · 2023-01-27T13:06:52Z

Solves #309

h-arlt · 2023-02-10T15:24:03Z

config/reportgenerator.properties

+# 
+#   index ........ Numeric value for indexing.
+#
+#   percentile ... The percentage of values taken when calculating the moving average series.


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

h-arlt · 2023-02-10T15:26:02Z

config/reportgenerator.properties

 ## The percentage of values taken when calculating the moving average series.
 com.xceptance.xlt.reportgenerator.charts.movingAverage.percentageOfValues = 5

+###############################################################################
+#
+# Additional Moving Average Settings
+# 


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)

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?

See here for more input regarding this topic.

h-arlt · 2023-02-10T15:48:13Z

config/reportgenerator.properties

+#   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.


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.

h-arlt · 2023-02-10T15:51:03Z

src/main/java/com/xceptance/xlt/report/ReportGeneratorConfiguration.java

@@ -117,7 +121,7 @@ public class ReportGeneratorConfiguration extends AbstractConfiguration implemen
         */
        public double parameter;
    }
-
+    


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

h-arlt · 2023-02-10T15:55:08Z

src/main/java/com/xceptance/xlt/report/ReportGeneratorConfiguration.java

+    private int movingAveragePoints;
+
+    private List<MovingArerage> movingAverageEntries;


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

h-arlt · 2023-02-10T15:57:51Z

src/main/java/com/xceptance/xlt/report/ReportGeneratorConfiguration.java

@@ -365,6 +379,7 @@ public ReportGeneratorConfiguration(Properties xltProperties, final File overrid
        {
            testReportsRootDir = new File(homeDirectory, testReportsRootDir.getPath());
        }
+        this.movingAveragePoints = 0;


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

h-arlt · 2023-02-13T08:16:24Z

src/main/java/com/xceptance/xlt/report/util/MovingArerage.java

+    private int intValue = 0;
+
+    /**
+     * The value used for the time.
+     */
+    private long longValue = 0;
+
+    /**
+     * Given property value for the time. 
+     */
+    private String timeString = "";
+
+    /**
+     * Current used type for moving average.
+     */
+    private MovingAverages type;


In case the values are not expected to change (as the absence of any setter method implies) once they have been set via c'tor, they should be declared as final.
And, there is no need to initialize fields with Java defaults (int and long are initialized implicitly w/ 0).

BTW do we really need two fields here? IMO having one field of type long would suffice too.

h-arlt · 2023-02-13T08:27:58Z

src/main/java/com/xceptance/xlt/report/util/JFreeChartUtils.java

+        int percentage = (int) Math.floor((value / requestCount) * 100);
+        // samples for chart data items
+        final int samples = Math.max(2, series.getItemCount() * percentage / 100);


This doesn't seem to be correct as it uses a percentage of values instead of a fixed amount. Maybe, I just don't get your intention here. But I'd expect something like the following

final int samples = Math.max(2, value);

We might also have to verify that value is not greater than requestCount.

h-arlt · 2023-02-13T08:32:17Z

src/main/java/com/xceptance/xlt/report/util/JFreeChartUtils.java

+        int percentage = (int) Math.floor((value / requestCount) * 100);
+        // samples for chart data items
+        final int samples = Math.max(2, series.getItemCount() * percentage / 100);
+        DecimalFormat decimalFormat = new DecimalFormat("0.##E0");


What about to made this a static field such it won't be created over and over again?

h-arlt · 2023-02-13T08:38:12Z

src/test/java/com/xceptance/xlt/report/providers/DummyReportProviderConfiguration.java

+    @Override
+    public List<MovingArerage> getAdditonalMovingAverages()
+    {
+        return Collections.emptyList();


Since Java 9, List interface provides method of that returns type-safe immutable collections including a stable reference to empty list. Thus, there is no need for Collections.emptyList() anymore.

Suggested change

return Collections.emptyList();

return List.of();

This is no CR, just FYI.

h-arlt · 2023-02-13T08:41:53Z

src/main/java/com/xceptance/xlt/report/util/JFreeChartUtils.java

@@ -85,7 +86,9 @@
        /**
         * blue, ...
         */
-        public static final ColorSet AVERAGES = new ColorSet(COLOR_MOVING_AVERAGE, COLOR_MEDIAN, COLOR_MEAN);
+        public static final ColorSet AVERAGES = new ColorSet(COLOR_MOVING_AVERAGE, COLOR_MEDIAN, COLOR_MEAN, COLOR_MOVING_AVERAGE_ADD_1,


To clarify: what if user configured more moving averages than colors available?

Xceptance#309 Implemented functionality.

5f0f2c5

Glenmorangie added enhancement New feature or request report Change to report or reporting. labels Jan 27, 2023

Xceptance#309 adjustementes for more user friendly handling.

af325ed

h-arlt reviewed Feb 10, 2023

View reviewed changes

h-arlt requested changes Feb 13, 2023

View reviewed changes

h-arlt self-assigned this Feb 13, 2023

h-arlt linked an issue Feb 13, 2023 that may be closed by this pull request

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Glenmorangie commented Jan 27, 2023 •

edited by h-arlt

Loading

h-arlt Feb 10, 2023

h-arlt Feb 10, 2023

h-arlt Feb 13, 2023

jowerner Feb 21, 2023

h-arlt Feb 10, 2023

h-arlt Feb 10, 2023

h-arlt Feb 10, 2023

h-arlt Feb 10, 2023

h-arlt Feb 13, 2023

h-arlt Feb 13, 2023

h-arlt Feb 13, 2023

h-arlt Feb 13, 2023

h-arlt Feb 13, 2023

		private int movingAveragePoints;

		private List<MovingArerage> movingAverageEntries;

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

Are you sure you want to change the base?

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

Conversation

Glenmorangie commented Jan 27, 2023 • edited by h-arlt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glenmorangie commented Jan 27, 2023 •

edited by h-arlt

Loading