Add metric health schema to Entities #50

pavan-traceable · 2020-12-02T16:07:57Z

Add baseline for metrics for entities

Description

Baseline, lower bound and upper bound values for entity metrics and used gateway service implementation.

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

tim-mwangi · 2020-12-02T16:27:46Z

.../src/main/java/org/hypertrace/graphql/utils/metrics/gateway/MetricContainerMapConverter.java

      implements MetricContainer {

    private final Map<Duration, List<MetricInterval>> metricSeriesMap;

    ConvertedMetricContainer(
-        Map<MetricLookupMapKey, MetricAggregation> metricAggregationMap,
+        Map<MetricLookupMapKey, BaselineMetricAggregation> metricAggregationMap,


BaselineMetricAggregation implies something totally different from the existing MetricAggregation. Are you sure you want to do a replace/rename instead of an augmentation to the existing users of MetricAggregation with a different object of the type BaselineMetricAggregation?

@tim-mwangi I didn't understand. Can you please illustrate more ?

...java/org/hypertrace/graphql/utils/metrics/gateway/BaselineConvertedAggregationContainer.java

hypertrace-graphql-metric-schema/src/main/java/org/hypertrace/graphql/metric/schema/Health.java

...metric-schema/src/main/java/org/hypertrace/graphql/metric/schema/BaselineMetricInterval.java

...a/src/main/java/org/hypertrace/graphql/metric/schema/BaselineMetricAggregationContainer.java

...ric-schema/src/main/java/org/hypertrace/graphql/metric/schema/BaselineMetricAggregation.java

.../src/main/java/org/hypertrace/graphql/utils/metrics/gateway/MetricContainerMapConverter.java

...rc/main/java/org/hypertrace/graphql/utils/metrics/gateway/MetricAggregationMapConverter.java

...s/src/main/java/org/hypertrace/graphql/utils/metrics/gateway/MetricAggregationConverter.java

pavan-traceable · 2020-12-02T16:49:36Z

@tim-mwangi @aaron-steinfeld thanks for the quick review. This patch is not complete it needs refactoring i should have updated description well, i have just updated just to verify with you guys regarding schema changes. Since generics is not supported in graphql so want to check whether schema changes fine or not.

aaron-steinfeld · 2020-12-02T17:02:31Z

@tim-mwangi @aaron-steinfeld thanks for the quick review. This patch is not complete it needs refactoring i should have updated description well, i have just updated just to verify with you guys regarding schema changes. Since generics is not supported in graphql so want to check whether schema changes fine or not.

Just to clarify, generics can be used (and should be, for enforcing keeping objects in sync) - they just need to be reified in child interfaces so the reflection code can find the appropriate types.

* updates release workflow and gradle plugin versio * clean up * indentation * executor name * executor name * Update .circleci/config.yml Co-authored-by: Aaron Steinfeld <[email protected]> Co-authored-by: Aaron Steinfeld <[email protected]>

pavan-traceable · 2020-12-29T16:22:12Z

@aaron-steinfeld Please review the implementation and API changes. Some minor things pending will address along with review comments. Not able to find placeholder to calculate Health to metrics, please suggest the right place.

.circleci/config.yml

...ic-schema/src/main/java/org/hypertrace/graphql/metric/schema/BaselinedMetricAggregation.java

...e-graphql-entity-schema/src/main/java/org/hypertrace/graphql/entity/dao/EntityDaoModule.java

...hema/src/main/java/org/hypertrace/graphql/entity/dao/GatewayServiceEntityRequestBuilder.java

...s/src/main/java/org/hypertrace/graphql/utils/metrics/gateway/MetricAggregationConverter.java

...rc/main/java/org/hypertrace/graphql/utils/metrics/gateway/BaselineIntervalListContainer.java

aaron-steinfeld · 2020-12-29T22:53:00Z

...rc/main/java/org/hypertrace/graphql/utils/metrics/gateway/BaselineIntervalListContainer.java

+  private final MetricBaselineAggregationConverter baselineAggregationConverter;
+
+  @Inject
+  BaselineIntervalListContainer(MetricBaselineAggregationConverter baselineAggregationConverter) {


this looks like mostly copy paste from the existing interval list converter - how much of the logic could be shared? I expect most of it could go in a base class that is just parameterized with the specific impementation builders.

Will raise a jira and do this refactoring later.

aaron-steinfeld · 2020-12-29T22:56:03Z

...rc/main/java/org/hypertrace/graphql/utils/metrics/gateway/BaselineIntervalListContainer.java

+      this.endTime = intervalTimeRange.getEndTime();
+    }
+
+    // TODO Fix this


Assume this is due to the null below?

...rc/main/java/org/hypertrace/graphql/utils/metrics/gateway/BaselineIntervalListContainer.java

aaron-steinfeld · 2020-12-29T22:57:12Z

...main/java/org/hypertrace/graphql/utils/metrics/gateway/BaselineMetricSeriesMapConverter.java

+  public Single<Map<Duration, List<BaselineMetricInterval>>> convert(
+      List<MetricSeriesRequest> metricSeriesRequests,
+      Map<String, BaselineMetricSeries> baselineMetricSeriesMap) {
+    return Observable.fromIterable(metricSeriesRequests)


Same as intervallist, this looks like copy paste that could be shared.

As i said earlier will do refactoring in next iteration since it already became big patch.

aaron-steinfeld · 2020-12-29T23:03:40Z

@aaron-steinfeld Please review the implementation and API changes. Some minor things pending will address along with review comments. Not able to find placeholder to calculate Health to metrics, please suggest the right place.

Lots of comments, many of them nit/style. Can we please first format this code with google style (in 1 standalone commit), then merge in main (again standalone), then fix any compilation issues, since it's not even running in CI and I'm fairly sure it won't compile as is.

As to health, my recommendation would be to omit it (from the schema as well as the implementation). We need to be far more iterative with this, and we can easily add health in a later round - the scope of this is already very large and difficult to review.

jcchavezs · 2020-12-30T13:54:27Z

Lots of comments, many of them nit/style. Can we please first format this code with google style (in 1 standalone commit), then merge in main (again standalone), then fix any compilation issues, since it's not even running in CI and I'm fairly sure it won't compile as is.

My 2p in here. Could we add a GH action checking this by default? That way we avoid unrelated changes in PRs due to formatting. cc @JBAhire

As to health, my recommendation would be to omit it (from the schema as well as the implementation). We need to be far more iterative with this, and we can easily add health in a later round - the scope of this is already very large and difficult to review.

Yes please, this PR is already big. Maybe it is easier to break it up in small PRs? Otherwise reviewing this is almost impossible and it is easy to make mistakes. Please consider either merge smaller PRs against main or another feature branch as they are easier to review.

pavan-traceable · 2020-12-31T09:58:05Z

Compilation issues are because it depends on gateway-service which is not merged yet.

JBAhire · 2021-01-03T13:49:09Z

Related to formating on CI: hypertrace/document-store#24

Will incorporate this. Thanks @jcchavezs :)

pavan-traceable · 2021-01-04T04:37:49Z

@jcchavezs We can divide this PR into API and implementation, but API part is small compared to implementation. From next time i will try to push small PR's, if possible

…rtrace-graphql into master/feature/healthapi

pavan-traceable · 2021-01-07T13:01:03Z

...in/java/org/hypertrace/graphql/utils/metrics/gateway/MetricBaselineAggregationConverter.java

+    }
+  }
+
+  static class MetricBaselineAggregationDefaultInstance implements MetricBaselineAggregation {


@aaron-steinfeld Please suggest if this can be improved further.

My suggestion would be to remove the default one - if Baseline is null, convert should not be called, since that indicates no baseline object was either requested or returned from the server. The behavior I'd expect for null responses for requested data is described below.

Also to make MetricBaselineAggregationImpl a simple value class like most of our converted classes, handling the unpacking of it in the converter itself rather than the value class.
Example:

@lombok.Value @Accessors(fluent = true) private static class MetricBaselineAggregationImpl implements MetricBaselineAggregation { Double value; Double lowerBound; Double upperBound; }

In terms of nulls, we want the following behavior:
if I request an aggregation, but there's no data in the time range (this can only happen in the recently added, time agnostic mode):

// Request sum { value } // No data response { sum: { value: null } }

Extending that to baselines, I'd expect:

sum { value baseline { value lowerBound upperBound } } // No baseline response { sum: { value: 42 baseline: null // the object, not the contents } }

pavan-traceable · 2021-01-07T13:22:58Z

@aaron-steinfeld I have updated all comments. Please review.

...ty-schema/src/main/java/org/hypertrace/graphql/entity/dao/GatewayServiceEntityConverter.java

aaron-steinfeld · 2021-01-07T19:05:03Z

...l-entity-schema/src/main/java/org/hypertrace/graphql/entity/dao/GatewayServiceEntityDao.java

-        .flatMap(baselineRequest -> baselineProvider.makeRequest(request.resultSetRequest().context(), baselineRequest))
-        .flatMap(baselineResponse -> this.entityConverter.convert(request, serverResponse, baselineResponse))));
+        .flatMap(
+            serverRequest ->


Correct, it's not a ton of complexity (although triconverter doesn't appear to be defined anywhere in this PR, one of the reasons it doesn't compile), it just saves a level of nesting here. The reason the nesting is required is we're doing one operation at a time but not carrying the previous results with us - so using a result container both saves nesting and removes the need for the new converter interface. It's not required, but I do think it would be nice to remove the nesting one way or another to improve readability. Another way of doing that is just breaking out the calls into different methods rather than doing them inline. For example, creating a new method like makeBaselineAndEntityRequests

...c-schema/src/main/java/org/hypertrace/graphql/metric/request/MetricSeriesRequestBuilder.java

...ce-graphql-entity-schema/src/main/java/org/hypertrace/graphql/entity/health/BaselineDao.java

aaron-steinfeld · 2021-01-07T19:34:54Z

...in/java/org/hypertrace/graphql/utils/metrics/gateway/MetricBaselineAggregationConverter.java

+    }
+  }
+
+  static class MetricBaselineAggregationDefaultInstance implements MetricBaselineAggregation {


My suggestion would be to remove the default one - if Baseline is null, convert should not be called, since that indicates no baseline object was either requested or returned from the server. The behavior I'd expect for null responses for requested data is described below.

Also to make MetricBaselineAggregationImpl a simple value class like most of our converted classes, handling the unpacking of it in the converter itself rather than the value class.
Example:

@lombok.Value @Accessors(fluent = true) private static class MetricBaselineAggregationImpl implements MetricBaselineAggregation { Double value; Double lowerBound; Double upperBound; }

In terms of nulls, we want the following behavior:
if I request an aggregation, but there's no data in the time range (this can only happen in the recently added, time agnostic mode):

// Request sum { value } // No data response { sum: { value: null } }

Extending that to baselines, I'd expect:

sum { value baseline { value lowerBound upperBound } } // No baseline response { sum: { value: 42 baseline: null // the object, not the contents } }

.../src/main/java/org/hypertrace/graphql/utils/metrics/gateway/MetricContainerMapConverter.java

aaron-steinfeld · 2021-01-07T19:51:47Z

...n/java/org/hypertrace/graphql/utils/metrics/gateway/BaselineMetricIntervalListContainer.java

+import org.hypertrace.graphql.metric.schema.BaselineMetricInterval;
+import org.hypertrace.graphql.metric.schema.BaselinedMetricAggregation;
+import org.hypertrace.graphql.metric.schema.MetricBaselineAggregation;
+


should be no gap in imports - please use the google formatter and update your IDE! We should enforce this in CI too, but all of our projects follow the same formatter regardless.

Yes did that sometime back, even though i formatted entire code using google formatter, it didnt deleted that gap. Anyway i am deleting myself.

Hmm - did you use the intellij format config or the plugin? I know the format file isn't able to capture the full style, the plugin might have shortcomings too - I use both, but that feels excessive. We'll revisit this with the push to enforce this in CI (and maybe add a gradle task)

...a/src/main/java/org/hypertrace/graphql/metric/schema/BaselineMetricAggregationContainer.java

...metric-schema/src/main/java/org/hypertrace/graphql/metric/schema/BaselineMetricInterval.java

...n/java/org/hypertrace/graphql/utils/metrics/gateway/BaselineMetricIntervalListContainer.java

pavan-traceable · 2021-01-08T12:55:59Z

@aaron-steinfeld I have addressed all your comments, only thing pending is removing nesting inline in GatewayServiceEntityDao which can be done in next iteration. UI team is already blocked on this. Lets push this, but before that we should merge hypertrace/hypertrace-core-graphql#43

pavan-traceable · 2021-01-08T14:18:52Z

I have updated core-graphql still it is showing conflicts.

aaron-steinfeld · 2021-01-08T14:14:57Z

...rc/main/java/org/hypertrace/graphql/utils/metrics/gateway/MetricAggregationMapConverter.java


    @Override
-    public Double value() {
-      return metricAggregation.value();
+    public MetricBaselineAggregation baseline() {


These two methods shouldn't be needed with the lombok annotations above

aaron-steinfeld · 2021-01-08T14:24:03Z

...in/java/org/hypertrace/graphql/utils/metrics/gateway/MetricBaselineAggregationConverter.java

-    public Double value() {
-      return null;
-    }
+    static final MetricBaselineAggregation EMPTY =


So this would end up with null values rather than a null baseline object - we can go with that and revisit

aaron-steinfeld · 2021-01-08T14:27:59Z

I have updated core-graphql still it is showing conflicts.

You'll need to rebase this repo off main, then update core-graphql. Once that's done, I think we're good at a good iteration point.

…master/feature/healthapi

codecov · 2021-01-08T15:18:57Z

Codecov Report

Merging #50 (ef9fc9b) into main (41dd991) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main      #50   +/-   ##
=========================================
  Coverage     80.55%   80.55%           
  Complexity        2        2           
=========================================
  Files             2        2           
  Lines            36       36           
=========================================
  Hits             29       29           
  Misses            7        7

Flag	Coverage Δ	Complexity Δ
unit	`80.55% <ø> (ø)`	`0.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41dd991...ef9fc9b. Read the comment docs.

pavan-traceable · 2021-01-08T15:19:51Z

@aaron-steinfeld All checks have passed.

github-actions · 2021-01-08T15:28:59Z

Unit Test Results

1 files ±0 1 suites ±0 1s ⏱️ -1s
1 tests ±0 1 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 3d0decb. ± Comparison against base commit 41dd991.

Add metric health schema to Entities

9e8a98f

pavan-traceable requested a review from a team as a code owner December 2, 2020 16:07

tim-mwangi reviewed Dec 2, 2020

View reviewed changes

aaron-steinfeld reviewed Dec 2, 2020

View reviewed changes

Pavan Kumar Kolamuri and others added 6 commits December 18, 2020 16:56

Addressed comments and added baseline provider

72a4cff

Addressed comments and added baseline provider

6c1c4d7

Addressed comments and added baseline provider

aa09fd5

allows tagOverride for helm charts (#48)

cdcf9fe

chore: bump dependencies (#49)

771b146

pavan-traceable requested a review from buchi-busireddy December 18, 2020 14:43

Pavan Kumar Kolamuri added 10 commits December 29, 2020 19:48

Add Implementation for Baseline

9d0932b

Removed unwanted changes

b842fca

Removed unwanted changes

3ff8eef

Removed unwanted changes

e6a6c19

Removed unwanted changes

90aa5e0

Removed unwanted changes

c04e8e5

Removed unwanted changes

f37745b

Removed unwanted changes

878286b

Removed unwanted changes

200da5a

Remove unwanted changes

1480271

aaron-steinfeld reviewed Dec 29, 2020

View reviewed changes

Pavan Kumar Kolamuri and others added 3 commits December 30, 2020 16:14

Addressed comments

81da7a6

Updated core graphql

895794c

Merge branch 'main' into master/feature/healthapi

ef8bfa5

Addressed comments

6f97e36

Merge branch 'main' into master/feature/healthapi

224ae6d

Pavan Kumar Kolamuri added 2 commits January 4, 2021 13:16

add converter for baseline series

d05221e

Merge branch 'master/feature/healthapi' of github.com:hypertrace/hype…

4f6baf8

…rtrace-graphql into master/feature/healthapi

pavan-traceable requested a review from jake-bassett January 4, 2021 17:17

Pavan Kumar Kolamuri added 3 commits January 7, 2021 17:21

Removed Optionals as an arguments

0081cdb

Fixed unused imports

66b6e29

Fixed comments

944ec88

pavan-traceable commented Jan 7, 2021

View reviewed changes

pavan-traceable requested a review from aaron-steinfeld January 7, 2021 17:08

aaron-steinfeld reviewed Jan 7, 2021

View reviewed changes

Pavan Kumar Kolamuri added 2 commits January 8, 2021 18:18

Addressed comments

0cc8037

Fix file names

ad87b59

Updated core graphql

0225807

aaron-steinfeld reviewed Jan 8, 2021

View reviewed changes

Merge branch 'main' of github.com:hypertrace/hypertrace-graphql into …

ef9fc9b

…master/feature/healthapi

This comment has been minimized.

Sign in to view

aaron-steinfeld approved these changes Jan 8, 2021

View reviewed changes

pavan-traceable merged commit 3d0decb into main Jan 8, 2021

pavan-traceable deleted the master/feature/healthapi branch January 8, 2021 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metric health schema to Entities #50

Add metric health schema to Entities #50

pavan-traceable commented Dec 2, 2020 •

edited

Loading

tim-mwangi Dec 2, 2020

pavan-traceable Dec 18, 2020

pavan-traceable commented Dec 2, 2020

aaron-steinfeld commented Dec 2, 2020

pavan-traceable commented Dec 29, 2020

aaron-steinfeld Dec 29, 2020

pavan-traceable Dec 30, 2020

aaron-steinfeld Dec 29, 2020

aaron-steinfeld Dec 29, 2020

pavan-traceable Jan 7, 2021

aaron-steinfeld commented Dec 29, 2020

jcchavezs commented Dec 30, 2020

pavan-traceable commented Dec 31, 2020

JBAhire commented Jan 3, 2021

pavan-traceable commented Jan 4, 2021

pavan-traceable Jan 7, 2021

aaron-steinfeld Jan 7, 2021

pavan-traceable commented Jan 7, 2021

aaron-steinfeld Jan 7, 2021

aaron-steinfeld Jan 7, 2021

aaron-steinfeld Jan 7, 2021

pavan-traceable Jan 8, 2021

aaron-steinfeld Jan 8, 2021

pavan-traceable commented Jan 8, 2021

pavan-traceable commented Jan 8, 2021

aaron-steinfeld Jan 8, 2021

aaron-steinfeld Jan 8, 2021

aaron-steinfeld commented Jan 8, 2021

codecov bot commented Jan 8, 2021

This comment has been minimized.

pavan-traceable commented Jan 8, 2021

github-actions bot commented Jan 8, 2021

Add metric health schema to Entities #50

Add metric health schema to Entities #50

Conversation

pavan-traceable commented Dec 2, 2020 • edited Loading

Description

Testing

Checklist:

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavan-traceable commented Dec 2, 2020

aaron-steinfeld commented Dec 2, 2020

pavan-traceable commented Dec 29, 2020

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

aaron-steinfeld commented Dec 29, 2020

jcchavezs commented Dec 30, 2020

pavan-traceable commented Dec 31, 2020

JBAhire commented Jan 3, 2021

pavan-traceable commented Jan 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavan-traceable commented Jan 7, 2021

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

pavan-traceable commented Jan 8, 2021

pavan-traceable commented Jan 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-steinfeld commented Jan 8, 2021

codecov bot commented Jan 8, 2021

Codecov Report

This comment has been minimized.

pavan-traceable commented Jan 8, 2021

github-actions bot commented Jan 8, 2021

Unit Test Results

pavan-traceable commented Dec 2, 2020 •

edited

Loading