-
Notifications
You must be signed in to change notification settings - Fork 13k
regression: save statistics on version change causes air-gapped state assumption #38298
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
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdded a version-based freshness check to sendUsageReport (uses Info.version) to trigger new statistics when version differs or data is older than 24 hours; removed statistics.updateDeploymentData and its startup invocation. Changes
Sequence Diagram(s)sequenceDiagram
participant Send as SendUsageReport
participant Stats as StatisticsStore
participant Local as LocalSave
participant Collector as RemoteCollector
Send->>Stats: findLast()
alt last exists and last.version == currentVersion and <24h
Send->>Send: return existing token
else
Send->>Send: generate new statistics
Send->>Local: save locally
alt collector configured
Send->>Collector: post statistics
Collector-->>Send: collector response
end
Send-->>Stats: save new stats (with currentVersion and token)
Send-->>Send: return token
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.1.0 #38298 +/- ##
================================================
Coverage ? 70.71%
================================================
Files ? 3142
Lines ? 108948
Branches ? 19608
================================================
Hits ? 77038
Misses ? 29907
Partials ? 2003
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
9023259 to
d6e31c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 4 files
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Proposed changes (including videos or screenshots)
A regression was inserted with this PR trying to fix the issue of versions mismatching when upgrading/downgrading the server version. That PR introduced a function that creates a copy of the last statistics record when the onVersionChange hook was triggered, but modifying the data related to deployment and versions. The problem was that Rocket.Chat sends workspace statistics to the cloud and it sends back data in a string token that is saved in statistics as statsToken and this token was being copied from the latest record to the new one but with an updated createdAt property, so the sendUsageReport function from meteor/app/statistics/server/functions/sendUsageReport.ts in some specific cases where the version is constantly upgraded/downgraded (in less than 24 hours), it wouldn't create a new statistics record with the statsToken given from cloud, and therefore the system would assume that the workspace is air-gapped.
Proposed solution:
Issue(s)
CORE-1736 : Statistics token timestamp stagnation causing false air-gapped state detection
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.