Skip to content
This repository was archived by the owner on Dec 23, 2023. It is now read-only.

Remove min/max from Distribution.#1542

Merged
songy23 merged 2 commits intocensus-instrumentation:masterfrom
bogdandrutu:minamx
Nov 2, 2018
Merged

Remove min/max from Distribution.#1542
songy23 merged 2 commits intocensus-instrumentation:masterfrom
bogdandrutu:minamx

Conversation

@bogdandrutu
Copy link
Copy Markdown
Contributor

No description provided.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 2, 2018

Codecov Report

Merging #1542 into master will decrease coverage by 0.04%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1542      +/-   ##
============================================
- Coverage     82.93%   82.89%   -0.05%     
  Complexity     1658     1658              
============================================
  Files           251      251              
  Lines          7719     7711       -8     
  Branches        754      748       -6     
============================================
- Hits           6402     6392      -10     
- Misses         1097     1099       +2     
  Partials        220      220
Impacted Files Coverage Δ Complexity Δ
.../opencensus/implcore/stats/MutableAggregation.java 91.03% <ø> (-0.69%) 0 <0> (ø)
...main/java/io/opencensus/stats/AggregationData.java 97.91% <91.66%> (+0.18%) 0 <0> (ø) ⬇️
...census/implcore/trace/export/SpanExporterImpl.java 90.41% <0%> (-2.74%) 8% <0%> (ø)

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 0aef639...5bb6721. Read the comment docs.

Copy link
Copy Markdown
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this to CHANGELOG.

- The histogram bucket boundaries (`BucketBoundaries`) and values (`Count` and `Sum`) are no longer
supported for negative values. The Record API drops the negative `value` and logs the warning.
This could be a breaking change if you are recording negative value for any `measure`.
- Remove support for min/max in the stats Distribution to make it compatible with Metrics.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also consider to change mean -> sum in stats Distribution to make it compatible with Metrics Distribution. ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please file an issue for that. It should be in a separate PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants