Refactored MetricsDefinition to use a MetricBuilder and be based off a super class#1
Refactored MetricsDefinition to use a MetricBuilder and be based off a super class#1
Conversation
johnjang
left a comment
johnjang
left a comment
There was a problem hiding this comment.
nice work so far! Left some questions in the file.
| .subList( | ||
| Constants.MAX_DATAPOINTS_PER_METRIC, | ||
| metric.getValues().size()))); | ||
| if (metric instanceof MetricDefinition) { |
There was a problem hiding this comment.
Personally, I'm not a big fan of instanceof and doesn't feel objected oriented in this specific case. If we add two more types of metrics in the future, this section may get harder to read and maintain.
I wonder if we can abstract this out into its own class.
| class MetricDefinition { | ||
| @NonNull | ||
| /** Abstract immutable (except for name) class that all Metrics are based on. */ | ||
| abstract class Metric { |
There was a problem hiding this comment.
Would prefer to have this in a separate file Metric.java to be honest instead of being coupled together in MetricBuilder.java file.
| } else if (metric instanceof MetricDefinitionBuilder) { | ||
| List<Double> values = ((MetricDefinitionBuilder) metric).getValues(); | ||
| targetMembers.put( | ||
| metric.getName(), values.size() == 1 ? values.get(0) : values); | ||
| } |
There was a problem hiding this comment.
is this else if statement necessary? if metric is instanceof MetricDefinitionBuilder, implicitly it is instanceof MetricDefinition already so wouldn't line 74 never get executed?
lksfrnz
force-pushed
the
MetricsRefactor
branch
2 times, most recently
from
b299268 to
9fd3833
Compare
|
Testing notes about 9fd3833: All but the resolveEnvironment tests passed which didn't pass on my machine before these changes, and these changes should not affect those tests. Also tested changes using the lambda example and the metrics were logged to CWL as expected |
| values.add(value); | ||
| } | ||
| /** @return a simplified representation of the values of this metric. */ | ||
| protected abstract Object getSimplifiedValues(); |
There was a problem hiding this comment.
qq: How would simplified values look like for histogram/stats?
There was a problem hiding this comment.
Statistics cannot be simplified so it would be the same as getValues(). For histograms if there are only 1 or 2 distinct values they would each be their own bucket as per the SEH1 algorithm implementation I am following.
There was a problem hiding this comment.
I think I'll rename this function to getPublishingValues() or something similar, to imply that it is getting values in the format that they will be published in.
There was a problem hiding this comment.
Updated name to getFormattedValues()
lksfrnz
force-pushed
the
MetricsRefactor
branch
2 times, most recently
from
df894a1 to
f59577b
Compare
3 participants
Refactored the MetricDefinition to inherit a Metric super class which can be inherited by other type of metrics (coming soon). Also refactored MetricDefinition to be immutable and created a MetricDefinitionBuilder which behaves similar to how MetricDefinition used to.
Naming is also handled using a setter instead of on creation for simplicity in upcoming metric changes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.