Skip to content

Add monitoring API for per-issuer validation statistics #182

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.

Sign up for GitHub

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

Jump to bottom
Merged
bbockelm merged 6 commits into from
Dec 10, 2025
Merged

Add monitoring API for per-issuer validation statistics #182

bbockelm merged 6 commits into from
Dec 10, 2025

Conversation

Copy link
Contributor

Copilot AI commented Dec 8, 2025 •
edited
Loading

Monitoring API Implementation - Complete ✅

Successfully implemented a monitoring API in the C interface to track per-issuer validation statistics with all code review feedback addressed.

Latest Changes

  • Code formatting: Applied clang-format to address all linter issues

Changes from Code Review

  • Exception hierarchy: Added IssuerLookupException and TokenExpiredException for proper error classification
  • Total time tracking: Changed from average to total validation time in seconds (base unit) for Prometheus compatibility
  • Named constants: Use MAX_SELECT_TIMEOUT_US constant instead of magic number for maintainability
  • Proper thread safety: All statistics properly protected by mutex, no race conditions
  • Code formatting: Properly formatted per clang-format rules

Features Implemented

  • ✅ Per-issuer tracking of successful/unsuccessful validations
  • ✅ Tracks total validation time in seconds (cumulative across all validations)
  • ✅ Tracks expired tokens via TokenExpiredException
  • ✅ Tracks failed issuer lookups via IssuerLookupException with DDoS protection (limits to 100 entries)
  • ✅ Returns statistics as JSON string
  • ✅ Includes reset functionality
  • ✅ Thread-safe with proper mutex protection

API

// Get statistics as JSON
char *json_out = NULL;
char *err_msg = NULL;
scitoken_get_monitoring_json(&json_out, &err_msg);
// Returns: {"issuers": {"https://example.org": {"successful_validations": 42, 
//           "unsuccessful_validations": 3, "expired_tokens": 1, 
//           "total_validation_time_s": 125.5}}, 
//           "failed_issuer_lookups": {"https://invalid.org": 5}}

// Reset all statistics
scitoken_reset_monitoring_stats(&err_msg);

Implementation Details

  • MonitoringStats singleton (src/scitokens_monitoring.cpp): Thread-safe statistics collection with mutex protection
  • Exception-based error classification: IssuerLookupException and TokenExpiredException instead of string matching
  • Validator integration: All verify() methods track timing with 50ms select timeout for responsiveness
  • DDoS mitigation: Failed issuer lookup tracking limited to 100 entries with pruning
  • Output sanitization: Issuer names truncated to 256 chars in JSON output

Statistics Tracked

Per issuer:

  • Successful/unsuccessful validation counts
  • Expired token count (detected via TokenExpiredException)
  • Total validation time in seconds (cumulative)

Global:

  • Failed issuer lookup counts (network/DNS errors via IssuerLookupException)

Test Programs

  • scitokens-test-monitoring: Basic API verification
  • scitokens-test-monitoring-comprehensive: DDoS protection and validation flow testing
Original prompt

This section details on the original issue you should resolve

<issue_title>Monitoring API</issue_title>
<issue_description>Create a new monitoring API in the C interface to the library. Potentially it should just return a JSON string that is an object with arbitrary monitoring statistics.

At the minimum, we should track statistics per-issuer (time spent per issuer, successful validations, unsuccessful validations, expirations, etc). For failed issuer lookup, we should make sure to avoid resource exhaustion DDoS attacks given attackers can generate an arbitrary number of issuers.</issue_description>

Comments on the Issue (you are @copilot in this section)

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Add new monitoring API to C interface Add monitoring API for per-issuer validation statistics Dec 8, 2025
Copilot AI requested a review from bbockelm December 8, 2025 23:01
github-actions[bot]
github-actions bot reviewed Dec 9, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

lint

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 19 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 27 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 30 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 33 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 36 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 39 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 55 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 57 in e27ae90

int result = scitoken_deserialize(expired_token.c_str(), &token, nullptr, &err_msg);

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 59 in e27ae90

std::cout << "Token deserialization/validation failed (expected): "

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 69 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 71 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Lines 73 to 74 in e27ae90

std::cout << "\n--- Test 2: Testing DDoS protection with multiple invalid issuers ---\n";

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 81 in e27ae90

scitoken_deserialize(fake_token.c_str(), &temp_token, nullptr, &err_msg);

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 87 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 89 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 100 in e27ae90

[lint] reported by reviewdog 🐶

scitokens-cpp/src/test_monitoring_comprehensive.cpp

Line 102 in e27ae90

bbockelm
bbockelm requested changes Dec 9, 2025
View reviewed changes
Copy link
Contributor

@bbockelm bbockelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - please address the first round of changes from my review and from the linter.

Copy link
Contributor Author

Copilot AI commented Dec 9, 2025 •
edited
Loading

@copilot - please address the first round of changes from my review and from the linter.

Addressed all review feedback in commit ed86d9a:

  • Changed to total time in seconds (not average in milliseconds)
  • Added exception hierarchy (IssuerLookupException, TokenExpiredException)
  • Refactored to get issuer stats once and update periodically (50ms intervals)
  • Removed string introspection for error classification

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • demo.scitokens.org
    • Triggering command: ./scitokens-test-monitoring-comprehensive ./scitokens-test-monitoring-comprehensive /tmp/ccWCbL3Z.s (dns block)
    • Triggering command: ./scitokens-test-monitoring-comprehensive ./scitokens-test-monitoring-comprehensive (dns block)
    • Triggering command: ./scitokens-test-monitoring-comprehensive ./scitokens-test-monitoring-comprehensive /tmp/cc8hb58q.s (dns block)
  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from bbockelm December 9, 2025 02:57
@bbockelm
Copy link
Contributor

bbockelm commented Dec 9, 2025

@copilot - address linter requests.

Copy link
Contributor Author

Copilot AI commented Dec 9, 2025 •
edited
Loading

@copilot - address linter requests.

Applied clang-format to all modified source files to address linter violations. Fixed in commit 6126b35.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • demo.scitokens.org
    • Triggering command: ./scitokens-test-monitoring-comprehensive ./scitokens-test-monitoring-comprehensive /tmp/ccnoLXci.s NDEBUG /home/REDACTED/work/scitokens-cpp/scitokens-cpp/src/scitokens_monitoring.cpp (dns block)
  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@bbockelm bbockelm force-pushed the copilot/add-monitoring-api branch from 6126b35 to d4e7332 Compare December 9, 2025 17:41
@bbockelm
Apply clang-format to fix linter issues
181ce60 
- Fix include order in test_monitoring_comprehensive.cpp
- Fix trailing whitespace in multiple files
- Fix line wrapping for long lines in test files
@bbockelm
Improve monitoring tests with counter validation and failure mechanisms
48672e6 
- Move monitoring tests from src/ to test/ directory
- Add MonitoringStats helper class to parse JSON and validate counters
- Add integration tests for monitoring API:
  - MonitoringCountersIncrease: Verify counters increase after validation
  - MonitoringFailedIssuerLookup404: Test 404 response handling
  - MonitoringFailedDNSLookup: Test DNS lookup failure
  - MonitoringFailedTCPConnection: Test TCP connection failure (port 1)
  - MonitoringDurationTracking: Verify duration increases over validations
- Fix duration unit from nanoseconds to seconds in verify_async_continue
- Update CMakeLists.txt to build monitoring tests with gtest
@bbockelm
Refactor monitoring: periodic duration updates in blocking verify()
ac2d80d 
- Add sync/async validation started counters (sync_validations_started,
  async_validations_started) separate from result counters
- Add sync/async duration tracking (sync_total_time_ns, async_total_time_ns)
- Update blocking verify() to track sync starts and update duration every
  50ms via select timeout, only calling verify_async_continue on actual I/O
- Update verify_async() to track async starts
- Add m_is_sync flag to AsyncStatus to prevent double-counting
- Add MAX_SELECT_TIMEOUT_US constant (50ms) for periodic checks
- Update get_json() to output new sync/async fields
- Update tests to parse new JSON fields
@bbockelm
Add periodic monitoring file output with thread-safe configuration
cb3e2e0 
- Add monitoring.file config option to enable periodic JSON file output
- Add monitoring.file_interval_s config option (default 60s)
- Use atomic flag for fast-path check in hot verify() path
- Use mutex-protected string for actual file path access
- Write to temp file then atomic rename for safe updates
- Silently ignore write errors (best-effort monitoring)
- Integrate with existing scitoken_config_* API instead of dedicated functions
- Add tests for configuration and file output
@bbockelm
Apply clang-format to source files
87848a0
bbockelm
bbockelm approved these changes Dec 10, 2025
View reviewed changes
Copy link
Contributor

@bbockelm bbockelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@bbockelm bbockelm marked this pull request as ready for review December 10, 2025 14:20
@bbockelm bbockelm merged commit cb1463f into master Dec 10, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

+1 more reviewer

@bbockelm bbockelm bbockelm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bbockelm bbockelm

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Monitoring API

2 participants

@bbockelm