Make NTP clock synchronization configurable and default to China-reachable servers - #25
Merged
Merged
Conversation
The SDK always synchronized its clock against a fixed set of public-internet NTP pool hosts, with no supported way to point it elsewhere or turn it off. A deployment isolated from the public internet cannot reach those hosts, and some environments do not permit contacting them at all. Configuration.Builder.setNtpHosts(List<String>) now chooses the servers, and an empty list skips clock synchronization entirely. Events are then timestamped with the device clock, which is the fallback the SDK already applied whenever synchronization failed, so no other behaviour changes.
Clock synchronization ran against the 0.datadog.pool.ntp.org through 3.datadog.pool.ntp.org hosts. Those are frequently slow or unreachable from mainland China, where this SDK is predominantly deployed, so synchronization timed out and events were timestamped with the device clock instead. The defaults are now ntp.aliyun.com, ntp1.aliyun.com, time1.cloud.tencent.com and cn.pool.ntp.org: two cloud providers plus the community pool, so no single operator being unreachable stops the clock from synchronizing. Apps that need other servers select them with setNtpHosts. DatadogNtpEndpoint only ever held those four hosts and is replaced by the list itself, since an enum whose values are immediately flattened to strings adds a type without adding meaning.
The entry asserted the previous default servers were frequently unreachable. Measuring from a mainland-China host shows three of the four do answer, so the entry now reports what was measured: the new servers answer at stratum 2 within 12-40 ms, the previous ones at stratum 3 within 41-241 ms, and one of the four did not answer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The SDK synchronizes its clock over NTP so that events carry a server-corrected timestamp rather than a possibly-skewed device clock. That synchronization was hardwired: it always targeted
0.datadog.pool.ntp.orgthrough3.datadog.pool.ntp.org, and there was no supported way to point it elsewhere or turn it off.Two changes, one commit each:
Configuration.Builder.setNtpHosts(List<String>)chooses the servers. An empty list skips synchronization entirely — no Kronos clock is created andtimeProviderstays on the device clock, which is the same fallback the SDK already applied whenever synchronization failed. A deployment isolated from the public internet can now either point at an internal NTP server or opt out.The defaults become
ntp.aliyun.com,ntp1.aliyun.com,time1.cloud.tencent.com,cn.pool.ntp.org. The previous hosts are frequently slow or unreachable from mainland China, where this SDK is predominantly deployed, so synchronization would time out and events fell back to the device clock anyway. The new set spans two cloud providers plus the community pool, so no single operator being unreachable stops the clock from synchronizing.DatadogNtpEndpointexisted only to hold those four hosts and had its values immediately flattened to strings, so it is replaced by the list itself.Behaviour change
Apps that do not call
setNtpHostswill now contact the new default servers instead of the previous ones. Clock-synchronization semantics are otherwise unchanged, and failure still degrades gracefully to the device clock.API surface
+ fun setNtpHosts(List<String>): Builder— additive only;apiSurfaceanddd-sdk-android-core.apiare regenerated. Minor version bump to 0.6.0.Tests
ConfigurationBuilderTest— custom hosts are carried into the config; an empty list is preserved.CoreFeatureTest— hosts are passed through toCoreFeature(asserted alongside the other config passthroughs); an empty list leaveskronosClocknull andtimeProvideraDefaultTimeProvider.Configuration.Coreforgery always produces a non-empty host list, so the disabled case stays confined to the tests that target it.