-
Notifications
You must be signed in to change notification settings - Fork 140
CBG-4579: Add uint64 support to few metrics in sgw #7828
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
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.
Pull Request Overview
This PR adds uint64 support to sequence and timestamp-related metrics in Sync Gateway to prevent potential integer overflow issues that could cause negative values when using int64.
- Introduces
SgwUIntStattype andAtomicUIntutility to handle unsigned 64-bit integer statistics - Converts sequence-related metrics (assigned, reserved, released counts and last values) and cache sequence metrics to use
uint64instead ofint64 - Updates compaction timestamp metrics to use
uint64for Unix timestamp storage
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| base/util.go | Adds AtomicUInt struct with thread-safe operations for uint64 values |
| base/stats.go | Introduces SgwUIntStat type and updates stat definitions for sequences and timestamps to use uint64 |
| db/sequence_allocator.go | Removes int64 type conversions when setting sequence metrics |
| db/change_cache.go | Updates cache stat calls to use uint64 values directly |
| db/database_stats.go | Removes int64 conversion for HighSeqCached metric |
| db/background_mgr_tombstone_compaction.go | Adds uint64 conversion for Unix timestamp |
| db/background_mgr_attachment_compaction.go | Adds uint64 conversion for Unix timestamp |
| db/sequence_allocator_test.go | Updates test assertions to expect uint64 values |
| db/database_test.go | Updates test assertions to expect uint64 values |
| db/crud_test.go | Updates test assertions to expect uint64 values |
| db/change_cache_test.go | Updates test assertions to expect uint64 values |
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.
Very small nits on this.
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.
Just some fiddly nits.
| return wrappedStat, nil | ||
| } | ||
|
|
||
| func (s *SgwUint64Stat) FormatString() string { |
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.
| func (s *SgwUint64Stat) FormatString() string { | |
| // FormatString returns a name of the stat type used for generating documentation. | |
| func (s *SgwUint64Stat) FormatString() string { |
This shows up in documentation https://docs.couchbase.com/sync-gateway/current/manage/stats-monitoring-prometheus.html and https://docs.couchbase.com/sync-gateway/current/manage/stats-monitoring-json.html
but curiously neither page use this value and it isn't generated by the generator that is used to update these pages.
| Format string `json:"-"` // The format of the value such as int, float, duration |
CBG-4579
Describe your PR here...
SgwUIntStatto useuint64instead ofint64for few metrics such as sequence and timestampsint64stats have a possibility of overflowing which would then make the number negativePre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/141/