Closed Tjofil closed 1 year ago
Thank you for making the changes @Tjofil. One high level comment I have is around naming of the sub-directories:
Would it make sense to:
collectors
-> statcollector
as it logically is a single piece for collecting operations stats.format
-> formatter
src/main/java/org/opensearch/performanceanalyzer/commons/stats/impl/vals/AggregateValue.java
to 1 level up -> src/main/java/org/opensearch/performanceanalyzer/commons/stats/vals/AggregateValue.java
@khushbr
With 2nd and 3rd I completely agree.
Besides StatsCollector
we also have PerformanceAnalyzerMetricsCollector
and Version
classes inside collectors that are to be used by all collectors, we can only move the StatsCollector
out if that's what we want but to me this current setup seems fine as other collectors will also live in the collectors package.
@khushbr With 2nd and 3rd I completely agree. Besides
StatsCollector
we also havePerformanceAnalyzerMetricsCollector
andVersion
classes inside collectors that are to be used by all collectors, we can only move theStatsCollector
out if that's what we want but to me this current setup seems fine as other collectors will also live in the collectors package.
This sounds good to me, let us retain the name of repo as collectors
2
This PR focuses on extracting three major components/component groups:
MetricsConfiguration
is a wrapper class around map that serves as configuration registry for all the collectors, both ones stored inPA
and ones stored inPA-RCA
repo. We don't do any initialization of the map entries from commons as that would require the class to know about collectors, which introduces dependencies from the two repos. Instead, we already have an initialization of this map from PA side which is done upon plugin class load, and the current initialization from inside class is going to be replaced with a symmetrical one that happens onPerformanceAnalyzerApp
class load.SampleAggregators
used to live asPerformanceAnalyzerApp
's static fields and Writer has to reach for this class in order to write to aggregators. We extract them in this PR, alongsideStatsReporter
by whom they are consumed, to a class calledCommonStats
which is visible to bothPA
andPA-RCA
repo and thus eliminatingPA
's need to know about the PA app class. They're still going to be initialized fromPA-RCA
code but used by both repos.StatsCollector
and everything necessary for it's functioning has also been moved to the repo, no changes were done to these classes. Note thatPA
andPA-RCA
have their own tests for different use cases ofStatsCollector
and these tests will remain inside their respective repos.Besides these extractions there is also a fix for Lychee link checker and tests for classes that had them inside their original repos.
There's also been a change to packages, generally small moves and flattening of the hierarchy and removal of
rca
subpackage as it was misleading in most of the cases.After this I'll publish two branches, one for PA and second for PA-RCA and make the changes to account for moving of the
MetricsConfiguration
, aggregators and StatsCollector so we can incrementally change the repos and make sure everything keeps working from their point of view.Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.