Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust outgoing activity timestamp #2208

Merged
merged 10 commits into from
Jul 23, 2019
Merged

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Jul 21, 2019

Fixes #1954.

Changelog Entry

  • Fix #1954. Estimate clock skew and adjust timestamp for outgoing activity, by @compulim in PR #2208

Description

If client clock is skewed significantly behind server clock (client clock < server clock for 5 seconds), the user-originated activity could be appears on top of the transcript momentarily until Direct Line Channel echo back the message with a corrected timestamp.

This is because the timestamp we temporarily assign to the user-originated activity is inaccurate. The insertion-sort put the outgoing activity before other activities received from the server. When server echo back the message, it will correct the timestamp.

This fix is to estimate the clock skew and applies to the outgoing activity.

Specific Changes

  • Modify postActivity.js
    • Piggyback client timestamp to channelData.clientTimestamp, to estimate the clock skew by comparing it with timestamp field
    • When patching timestamp field on outgoing activity, adjust it with the estimated clock skew
  • Add a new reducer, clockSkewAdjustment.js
    • Estimate adjustment on all incoming activity with channelData.clientTimestamp field

  • Testing Added

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 64.88% when pulling b3e04a2 on compulim:fix-1954 into cbee7cb on microsoft:master.

@compulim compulim merged commit 18a9a3b into microsoft:master Jul 23, 2019
@compulim compulim deleted the fix-1954 branch July 23, 2019 07:14
@corinagum
Copy link
Contributor

@compulim can we get another PR in with the changes I suggested?

@compulim compulim mentioned this pull request Jul 26, 2019
corinagum pushed a commit that referenced this pull request Jul 26, 2019
* Fix comments

* Add PR number
@compulim compulim mentioned this pull request Oct 25, 2019
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input message displayed (shortly) above bot message
4 participants