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

Fix time skew exceptions crashing disruptor thread #2071

Merged
merged 3 commits into from Jan 11, 2021

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented Jan 7, 2021

For now, we fix all known issues with time skew in Metric Views.

  • In the event of a detected time skew, we just throw away all our data. We really don't know if it's safe to push anything we have, and we cannot safely do any aggregation / report correct timestamps.
  • We add (the first?) tests for MutableViewData to ensure it does not throw when time rewinds, but instead drops recorded data points and resets itself.

Fixes #2068 (deemed to not have a workaround)

@google-cla google-cla bot added the cla: yes label Jan 7, 2021
@codecov-io
Copy link

codecov-io commented Jan 7, 2021

Codecov Report

Merging #2071 (abfb238) into master (6123b84) will decrease coverage by 0.01%.
The diff coverage is 60.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2071      +/-   ##
============================================
- Coverage     83.08%   83.06%   -0.02%     
  Complexity     2264     2264              
============================================
  Files           322      322              
  Lines         10557    10565       +8     
  Branches       1048     1049       +1     
============================================
+ Hits           8771     8776       +5     
- Misses         1448     1451       +3     
  Partials        338      338              
Impacted Files Coverage Δ Complexity Δ
.../io/opencensus/implcore/stats/MutableViewData.java 92.07% <60.00%> (-1.52%) 4.00 <0.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 6123b84...abfb238. Read the comment docs.

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM as a fix. I can approve if no one with more context on this reviews in the next fews days.

Is the IntervalMutableViewData only used for distributions?

@jsuereth jsuereth merged commit e3bf35f into census-instrumentation:master Jan 11, 2021
@jsuereth jsuereth deleted the wip-fix-time-skew branch January 11, 2021 14:50
jsuereth added a commit to jsuereth/opencensus-java that referenced this pull request Jan 11, 2021
…ation#2071)

* Add a fix to metric views when a time skew is detected rather than crashing the application/thread.
* Add test for time-rewind issues, and fix problems discovered.
jsuereth added a commit that referenced this pull request Jan 11, 2021
)

* Fix time skew exceptions crashing disruptor thread (#2071)

* Add a fix to metric views when a time skew is detected rather than crashing the application/thread.
* Add test for time-rewind issues, and fix problems discovered.

* Mark ContextUtils as public (but deprecated)  (#2072)

* Mark ContextUtils as public (but deprecated) to fix bincompat issues with 0.17.x->0.18.x
* Add an access mechanism for grpc context and update deprecation denotation.

* Update changelog with latest releases.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clock Skew(?) can cause the distruptor thread to crash
4 participants