Feature/matchy ntp#21
Merged
sempervent merged 25 commits intorelease/1.2.0from Apr 29, 2026
Merged
Conversation
added 13 commits
April 15, 2026 09:16
# Conflicts: # opensampl/mixins/collect.py
sempervent
reviewed
Apr 17, 2026
sempervent
reviewed
Apr 17, 2026
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
|
Co-authored-by: sempervent <jngrant9+git@gmail.com> Co-authored-by: MacFarland, Midgie <macfarlandmj@ornl.gov>
- Added a full 1.2.0 unreleased entry in OpenSAMPL/CHANGELOG.md:39
covering NTP support, dashboard/query hardening, jitter semantics, docs,
and
validation.
- Fixed the remote NTP collector to actually persist stratum in
OpenSAMPL/opensampl/vendors/ntp.py:429.
- Added NTP unit and load-path coverage in OpenSAMPL/tests/test_ntp.py:1
and OpenSAMPL/tests/test_load_data.py:1.
- Added geolocator coverage in OpenSAMPL/tests/test_geolocator.py:1 for
override, cache, lookup, and no-data paths without requiring live
network
access.
- Replaced the brittle pytest-postgresql integration harness with MockDB
in OpenSAMPL/tests/integration/conftest.py:1, and updated the
integration
assertions in OpenSAMPL/tests/integration/test_db_setup.py:1.
- Fixed the mock seeded default metric UUID bug in
OpenSAMPL/tests/utils/mockdb.py:1 and cleaned up stale vendor test
references in OpenSAMPL/tests/
test_vendors.py:1.
- Ensured test env defaults define ENABLE_GEOLOCATE in
OpenSAMPL/tests/conftest.py:1.
- Updated CI in OpenSAMPL/.github/workflows/tests.yml:1 to install
PostgreSQL 16 and PostGIS tooling so workflows can support
pytest-postgresql-style
environments when needed.
- Did a broad docs pass across OpenSAMPL/README.md:1,
OpenSAMPL/mkdocs.yaml:1, OpenSAMPL/docs/index.md:1, the getting-started
pages, CLI/server
guides, collection docs, API index/pages, and added
OpenSAMPL/docs/guides/ntp-extension.md:1. The main effect was to make
the docs true to the
current code: opensampl collect ntp ..., opensampl load NTP ..., current
server CLI behavior, current module paths, and current API nav.
---------
Co-authored-by: Grant, Josh <grantjn@ornl.gov>
Co-authored-by: MacFarland, Midgie <macfarlandmj@ornl.gov>
sempervent
approved these changes
Apr 29, 2026
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.
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 .