Draft
Conversation
added 13 commits
April 15, 2026 09:16
# Conflicts: # opensampl/mixins/collect.py
sempervent
reviewed
Apr 17, 2026
sempervent
reviewed
Apr 17, 2026
|
|
||
| probe_uuid = Column(String, ForeignKey("probe_metadata.uuid"), primary_key=True) | ||
| mode = Column(Text) | ||
| reference = Column(Boolean, comment="Is used as a reference for other probes") |
Contributor
There was a problem hiding this comment.
do we want to update all clock metadata to be used as a reference?
Contributor
Author
There was a problem hiding this comment.
We can get the information pretty easily from the tables, since any probe used as a reference will appear in the reference table.
I think i'll add a migration to add a view that joins our probe metadata against the reference table to make an easily accessible (and query-able) spot to get that info
sempervent
requested changes
Apr 22, 2026
Contributor
sempervent
left a comment
There was a problem hiding this comment.
- High: gen_api_key is unauthenticated, so enabling USE_API_KEY=true does not actually protect the backend. Any caller can hit /gen_api_key and mint a valid key without already having one, which defeats the access-control model entirely.
See opensampl/server/backend/main.py:309. - High: opensampl load ntp ... will fail through the shared loader path. BaseProbe.process_single_file() now instantiates probes as cls(input_file=..., chunk_size=..., **kwargs), but NtpProbe.init only accepts input_file and does not
accept chunk_size or **kwargs. That makes the new NTP parser incompatible with the standard load pipeline introduced in the same branch. See opensampl/vendors/base_probe.py:227 and opensampl/vendors/ntp.py:543. - Medium: collected probe files cannot be reloaded from a directory. _collect_and_save() writes files ending in .txt, but CollectMixin.filter_files() checks f.stem == ".txt". Path.stem never equals ".txt" for those files, so directory loads
will always filter them out. See opensampl/mixins/collect.py:141 and opensampl/mixins/collect.py:145. - Medium: local chrony collection stores phase offset as a string, not a float. _parse_chronyc_tracking() assigns m.group(1) directly to self.offset_s, and export_data() then serializes that value into the time-series artifact unchanged.
The downstream load path only keeps time/value, so this will persist string JSON for a metric declared as float, which is likely to break numeric queries/dashboards. See opensampl/vendors/ntp.py:190, opensampl/vendors/ntp.py:83, and
opensampl/load_data.py:137. - Medium: remote NTP collection has no usable default port. CollectConfig.port defaults to None, and remote mode passes that straight into NTPRemoteCollector(target_port=collect_config.port). A normal collect ntp --mode remote --host ...
run will therefore fail unless the caller knows to supply --port, instead of defaulting to standard NTP/123. See opensampl/vendors/ntp.py:511 and opensampl/vendors/ntp.py:653.
Contributor
|
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
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.
PR Progress
TODO:
DONE:
Summary
This PR adds first-class NTP support to OpenSAMPL and hardens the local/demo experience around initialization, loading, and visualization.
At a high level, it introduces:
What’s included
NTP vendor / probe support
ntp_metadatahandling and NTP-specific probe loading behaviorDashboard and query hardening
varchar = uuidfailure modesNTP jitter handling
Docs
1.2.0changelog entryWhy
Before this change:
After this change:
Notes
syncscope-at-homerepo.Validation
Verified locally with:
uv run pytestuv run ruff check .