Allow Webservice to Configure Metric Long Name #492

QubitPi · 2017-08-08T00:42:30Z

No description provided.

kevinhinterlong · 2017-08-08T13:25:56Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/metric/LogicalMetric.java

+     * @param calculation  Mapper for the metric
+     * @param logicalMetricInfo  Logical Metric info provider
+     */
+    public LogicalMetric(


It might be nice to implement this constructor and have everything else call this one.

You might also want to change this constructor LogicalMetricInfo(String name, String longName) to LogicalMetricInfo(String name, String description) since that's what LogicalMetric allows

Also, would we want to mark the other constructors with @Deprecated telling people to use a LogicalMetricInfo?

Let's go ahead and do this to push people toward the LogicalMetricInfo constructor

kevinhinterlong · 2017-08-08T13:30:24Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/MetricMaker.java

+     *
+     * @return the new logicalMetric
+     */
+    protected LogicalMetric makeInner(LogicalMetricInfo logicalMetricInfo, List<String> dependentMetrics) {


It looks like this was only implemented for AggregationAverageMaker, shouldn't it be done for all subclasses of MetricMaker?

Yes. AggregationAverageMaker is for experimenting. All subclasses will be modified

michael-mclawhorn · 2017-08-28T18:43:00Z

...c/main/java/com/yahoo/bard/webservice/data/config/metric/makers/AggregationAverageMaker.java

@@ -85,6 +86,21 @@ protected LogicalMetric makeInner(String metricName, List<String> dependentMetri
        return new LogicalMetric(outerQuery, NO_OP_MAPPER, metricName);
    }

+    @Override
+    protected LogicalMetric makeInner(LogicalMetricInfo logicalMetricInfo, List<String> dependentMetrics) {
+        // Get the Metric that is being averaged over


Would it make sense to proxy the other method into this one by building a default LogicalMetricInfo that is more or less backwards compatible?

@michael-mclawhorn Yes! This is be a brilliant idea! 👍

michael-mclawhorn

In addition to pushing people to use the new constructor via deprecations, perhaps add a test using the new makeInner.

michael-mclawhorn · 2017-08-28T19:04:33Z

Once you've addressed @kevinhinterlong comments I'll dismiss his review unless he jumps on here before then,

michael-mclawhorn · 2017-08-29T16:37:02Z

I don't understand why we didn't go through and add makeInner(LogicalMetricInfo) to all the subclasses of MetricMaker.

QubitPi · 2017-08-31T00:14:18Z

@michael-mclawhorn You are right. I'll add makeInner(LogicalMetricInfo) to all subclasses. The reason for not adding previously is to have an example implementation in AggregationAverageMaker.java for you to evaluate whether the implementation is good or not

garyluoex

Also, we need to change the api of MetricInstance and make make() to call make() with LogicalMetricInfo

garyluoex · 2017-09-11T15:35:35Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/MetricMaker.java

     */
-    protected abstract LogicalMetric makeInner(String metricName, List<String> dependentMetrics);
+    protected abstract LogicalMetric makeInner(LogicalMetricInfo logicalMetricInfo, List<String> dependentMetrics);


We should give it a default implementation, if not other peoples metric maker will break because they don't have this makeInner implemented.

garyluoex

Need to change this

garyluoex · 2017-09-18T14:26:56Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/MetricInstance.java

@@ -23,6 +24,7 @@
 */
 public class MetricInstance {

+    private final LogicalMetricInfo logicalMetricInfo;
    private final String metricName;


I don't think we need this anymore.

Need CHANGELOG

kevinhinterlong · 2017-09-18T19:08:01Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/MetricInstance.java

+        this.logicalMetricInfo = new LogicalMetricInfo(metricName.asName());
+        this.maker = maker;
+        this.dependencyMetricNames = new ArrayList<>();
+        for (FieldName fieldName : dependencyFields) {


Maybe make this streamy using Arrays.stream(...).map(...).collect(...)

kevinhinterlong · 2017-09-18T19:12:20Z

I only skimmed through it so you may want to get a third review/dismiss mine, but from what I saw 👍

michael-mclawhorn

I've suggested a tweak to the CHANGELOG, but otherwise, ready to merge.

michael-mclawhorn · 2017-09-18T19:15:55Z

CHANGELOG.md

+- [Allow Webservice to Configure Metric Long Name](https://github.com/yahoo/fili/pull/492)
+    * Logical metric needs more config-richness to not just configure metric name, but also metric long name,
+      description, etc. MetricInstance is now created by accepting a LogicalMetricInfo which contains all those
+      informations, instead of accepting a metric name.


which contains all these fields in addition to metric name.

QubitPi added NEED 2 REVIEWS REVIEWABLE labels Aug 8, 2017

yahoo deleted a comment Aug 8, 2017

kevinhinterlong requested changes Aug 8, 2017

View reviewed changes

michael-mclawhorn reviewed Aug 28, 2017

View reviewed changes

QubitPi force-pushed the allow-webservice-to-configure-metric-long-name branch 2 times, most recently from 05f277d to 646c40b Compare August 31, 2017 00:17

yahoo deleted a comment Aug 31, 2017

garyluoex requested changes Sep 11, 2017

View reviewed changes

yahoo deleted a comment Sep 12, 2017

Allow Webservice to Configure Metric Long Name

d809c93

QubitPi force-pushed the allow-webservice-to-configure-metric-long-name branch from 4540c0c to 1261d35 Compare September 15, 2017 16:26

yahoo deleted a comment Sep 15, 2017

Address @garyluoex's comments

d713509

QubitPi force-pushed the allow-webservice-to-configure-metric-long-name branch from 752b4cb to d713509 Compare September 15, 2017 16:34

yahoo deleted a comment Sep 15, 2017

garyluoex requested changes Sep 15, 2017

View reviewed changes

Address @garyluoex's comments

f8898fe

yahoo deleted a comment Sep 15, 2017

garyluoex approved these changes Sep 18, 2017

View reviewed changes

Address @garyluoex's comments

0f38e38

yahoo deleted a comment Sep 18, 2017

kevinhinterlong reviewed Sep 18, 2017

View reviewed changes

kevinhinterlong approved these changes Sep 18, 2017

View reviewed changes

michael-mclawhorn approved these changes Sep 18, 2017

View reviewed changes

Address comments

2178a63

QubitPi removed NEED 2 REVIEWS REVIEWABLE labels Sep 18, 2017

yahoo deleted a comment Sep 18, 2017

QubitPi added the MERGEABLE label Sep 18, 2017

QubitPi merged commit 706f8b5 into master Sep 18, 2017

QubitPi deleted the allow-webservice-to-configure-metric-long-name branch September 18, 2017 20:04

QubitPi mentioned this pull request Mar 6, 2018

[WIP] Fili 0.10.0 release notes #643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Webservice to Configure Metric Long Name #492

Allow Webservice to Configure Metric Long Name #492

QubitPi commented Aug 8, 2017

kevinhinterlong Aug 8, 2017 •

edited

Loading

kevinhinterlong Aug 8, 2017

michael-mclawhorn Aug 28, 2017

kevinhinterlong Aug 8, 2017

QubitPi Aug 30, 2017 •

edited

Loading

michael-mclawhorn Aug 28, 2017

QubitPi Aug 30, 2017 •

edited

Loading

michael-mclawhorn left a comment

michael-mclawhorn commented Aug 28, 2017

michael-mclawhorn commented Aug 29, 2017

QubitPi commented Aug 31, 2017

garyluoex left a comment

garyluoex Sep 11, 2017

garyluoex left a comment

garyluoex Sep 18, 2017

garyluoex Sep 18, 2017

kevinhinterlong Sep 18, 2017

kevinhinterlong commented Sep 18, 2017

michael-mclawhorn left a comment

michael-mclawhorn Sep 18, 2017

Allow Webservice to Configure Metric Long Name #492

Allow Webservice to Configure Metric Long Name #492

Conversation

QubitPi commented Aug 8, 2017

kevinhinterlong Aug 8, 2017 • edited 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

QubitPi Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

michael-mclawhorn left a comment

Choose a reason for hiding this comment

michael-mclawhorn commented Aug 28, 2017

michael-mclawhorn commented Aug 29, 2017

QubitPi commented Aug 31, 2017

garyluoex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyluoex left a 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

kevinhinterlong commented Sep 18, 2017

michael-mclawhorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinhinterlong Aug 8, 2017 •

edited

Loading

QubitPi Aug 30, 2017 •

edited

Loading

QubitPi Aug 30, 2017 •

edited

Loading