search

spotify / heroic

The Heroic Time Series Database
https://spotify.github.io/heroic/
Apache License 2.0
848 stars 109 forks source link

[Elasticsearch] Fix reporting of dropped by duplicate metrics #679

Closed lmuhlha closed 4 years ago

codecov[bot] commented 4 years ago

Codecov Report

Merging #679 into master will decrease coverage by 0.03%. The diff coverage is 25.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #679      +/-   ##
============================================
- Coverage     53.91%   53.87%   -0.04%     
+ Complexity     2964     2961       -3     
============================================
  Files           726      726              
  Lines         19423    19426       +3     
  Branches       1277     1279       +2     
============================================
- Hits          10472    10466       -6     
- Misses         8508     8514       +6     
- Partials        443      446       +3
Impacted Files Coverage Δ Complexity Δ
...ic/elasticsearch/AbstractElasticsearchBackend.java 64.28% <0.00%> (-4.95%) 3.00 <0.00> (ø)
...heroic/suggest/elasticsearch/SuggestBackendKV.java 80.16% <50.00%> (-0.98%) 61.00 <0.00> (-1.00)
...com/spotify/heroic/aggregation/simple/MaxBucket.kt 44.44% <0.00%> (-11.12%) 4.00% <0.00%> (-1.00%)
...com/spotify/heroic/aggregation/simple/MinBucket.kt 44.44% <0.00%> (-11.12%) 4.00% <0.00%> (-1.00%)

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 f2e6baa...3f6077c. Read the comment docs.

malish8632 commented 4 years ago

@lmuhlha just to make sure I didn't miss it. The biggest changes I see you removed try() for tracer.withSpan and rapped connection.doto to private doto. Is there anything else big I've missed?

lmuhlha commented 4 years ago

@malish8632 most of the changes are cosmetic when i was comparing the classes. The important changes that provide the fix are https://github.com/spotify/heroic/pull/679/files#diff-96ff9d0380bf9df05d420452e2a824eaR57 and https://github.com/spotify/heroic/pull/679/files#diff-1290b46b7a9f097ecbf07d898c73e788R681

The problem was that the version conflict format changed from the transport client to the rest client, and on top of that it appears to be nested inside another error. So it was being caught as a failure rather than dropped by duplicate.

I can revert the cosmetic changes if need be, I just left them because I liked the more similar formatting.