Skip to content
This repository has been archived by the owner on Mar 27, 2021. It is now read-only.

Remove legacy source input, optional metricType query input for testing #749

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

lmuhlha
Copy link
Member

@lmuhlha lmuhlha commented Jan 26, 2021

source was a legacy query option left over from when events were partially supported:
bc53307#diff-0e5dda61cc1e44c094fed3ce698715de1d3b11747d9f312df0fbf6a52f70f1fcR222

This PR gets rid of source as an input and essentially re-names it to metricType with a few updates.

Since end users don't care about what metricType is being used, Distributions can instead set metricType based on what aggregation (tdigest) is being used.

@lmuhlha lmuhlha requested a review from ao2017 January 26, 2021 23:40
@lmuhlha lmuhlha changed the title Remove legacy source input, move distribution type out of request input Remove legacy source input, adjust distribution type logic Jan 26, 2021
@ao2017
Copy link
Contributor

ao2017 commented Jan 27, 2021

@lmuhlha
There is a concept of metric_type downstream that is completely different from the metric_type as used in this PR.
metric_type upstream is ( histogram, counter, gauge ....)
metric_type in heroic is ( Point , Group, DistributionPoint ...)

Unfortunately, in heroic api this 2 concepts co-exist. In the query you can make the distinction by using "source". If you replace source with metric_type, how are we going to ensure this change doesn't break anything?

I wanted to check but this branch is not compiling. Check this class => /Users/adeleo/src/heroic/heroic/heroic-dist/src/test/java/com/spotify/heroic/LoggingMetricModule.kt: (36, 19) .

@lmuhlha lmuhlha force-pushed the sourcechange branch 2 times, most recently from 7956c01 to 26c8750 Compare January 27, 2021 20:40
@lmuhlha
Copy link
Member Author

lmuhlha commented Jan 27, 2021

@ao2017 I fixed a few issues, now I'm just having some trouble with the tests. When i step through the code, the check for the tDigest aggregation instance sets the metricType properly. I noticed that there are tests that tDigest still works with no aggregation set. Is that how this is supposed to work? I think I'm a bit confused on functionality.

@lmuhlha
Copy link
Member Author

lmuhlha commented Jan 27, 2021

@ao2017 as far as the naming, I just figured metricType matched what it is in Heroic. I don't think it will conflict with anything in semantic metrics. We can come up with another name if need be, i just think source was confusing as well.

@ao2017
Copy link
Contributor

ao2017 commented Jan 27, 2021

@ao2017 as far as the naming, I just figured metricType matched what it is in Heroic. I don't think it will conflict with anything in semantic metrics. We can come up with another name if need be, i just think source was confusing as well.

I am not sure why the name source was used :). If everything is working as expected I am good.

@ao2017
Copy link
Contributor

ao2017 commented Jan 27, 2021

@ao2017 I fixed a few issues, now I'm just having some trouble with the tests. When i step through the code, the check for the tDigest aggregation instance sets the metricType properly. I noticed that there are tests that tDigest still works with no aggregation set. Is that how this is supposed to work? I think I'm a bit confused on functionality.

Let's look into together. I will ping you.

@ao2017
Copy link
Contributor

ao2017 commented Jan 28, 2021

@ao2017 I fixed a few issues, now I'm just having some trouble with the tests. When i step through the code, the check for the tDigest aggregation instance sets the metricType properly. I noticed that there are tests that tDigest still works with no aggregation set. Is that how this is supposed to work? I think I'm a bit confused on functionality.

Let's look into together. I will ping you.

I checked. The refactoring on the consumer broke the ability to read distributionPoint from bigtable. It is causing query and consumer IT tests to fail. So the issue is not related to aggregation.

@lmuhlha lmuhlha force-pushed the sourcechange branch 6 times, most recently from 400d144 to 08ac6be Compare January 29, 2021 20:35
@lmuhlha lmuhlha marked this pull request as ready for review January 29, 2021 21:59
@lmuhlha lmuhlha changed the title Remove legacy source input, adjust distribution type logic Remove legacy source input, optional metricType query input for testing Jan 29, 2021
@lmuhlha lmuhlha force-pushed the sourcechange branch 3 times, most recently from b9c524d to d64efe3 Compare January 29, 2021 22:19
@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #749 (a18519f) into master (5e331a5) will decrease coverage by 0.00%.
The diff coverage is 76.59%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #749      +/-   ##
============================================
- Coverage     55.02%   55.02%   -0.01%     
+ Complexity     3154     3153       -1     
============================================
  Files           746      746              
  Lines         20386    20389       +3     
  Branches       1337     1339       +2     
============================================
+ Hits          11218    11219       +1     
- Misses         8681     8682       +1     
- Partials        487      488       +1     
Impacted Files Coverage Δ Complexity Δ
...main/java/com/spotify/heroic/metric/FullQuery.java 32.00% <0.00%> (ø) 7.00 <0.00> (ø)
...main/java/com/spotify/heroic/shell/task/Fetch.java 10.29% <0.00%> (ø) 2.00 <0.00> (ø)
...potify/heroic/metric/datastax/DatastaxBackend.java 27.27% <0.00%> (ø) 21.00 <0.00> (ø)
...potify/heroic/metric/bigtable/BigtableBackend.java 85.81% <50.00%> (ø) 61.00 <1.00> (ø)
...java/com/spotify/heroic/grammar/QueryExpression.kt 90.00% <66.66%> (ø) 10.00 <1.00> (ø)
...n/java/com/spotify/heroic/metric/QueryMetrics.java 81.81% <66.66%> (-0.80%) 4.00 <0.00> (ø)
...omponent/src/main/java/com/spotify/heroic/Query.kt 100.00% <100.00%> (ø) 7.00 <2.00> (ø)
...src/main/java/com/spotify/heroic/QueryBuilder.java 70.68% <100.00%> (+0.51%) 16.00 <2.00> (ø)
...c/main/java/com/spotify/heroic/metric/FetchData.kt 90.47% <100.00%> (ø) 3.00 <0.00> (ø)
...main/java/com/spotify/heroic/CoreQueryManager.java 75.53% <100.00%> (+0.31%) 10.00 <0.00> (ø)
... and 3 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 5e331a5...a18519f. Read the comment docs.

@lmuhlha lmuhlha merged commit 7a830df into master Feb 1, 2021
@delete-merged-branch delete-merged-branch bot deleted the sourcechange branch February 1, 2021 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants