Add include_stats parameter to status endpoint and reduce log verbosity#51
Merged
thiagoralves merged 1 commit intoDec 19, 2025
Merged
Conversation
- Add optional include_stats parameter to /api/status endpoint (default false) - Only fetch timing stats when include_stats=true, avoiding mutex acquisition - Remove verbose command logging from unix_socket.c (only log unrecognized commands) - This reduces log flooding and avoids unnecessary mutex contention on the critical PLC scan cycle when stats are not needed Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces log flooding from frequent STATUS and STATS commands by making timing statistics optional and removing verbose debug logging. The changes prevent unnecessary mutex acquisition during status polls, which could impact PLC scan cycle performance.
Key changes:
- Added
include_statsquery parameter to/api/statusendpoint (defaults tofalse) - Removed debug log statements for all recognized commands (PING, STATUS, STOP, START, STATS, DEBUG)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| webserver/app.py | Conditionally fetches timing stats only when include_stats=true parameter is provided |
| core/src/plc_app/unix_socket.c | Removes verbose debug logging for recognized commands and generic command receipt logs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 addresses log flooding from STATUS and STATS commands by:
Adding
include_statsparameter to/api/statusendpoint - The endpoint now only fetches timing stats wheninclude_stats=trueis explicitly passed. Default isfalse, which completely skips thestats_plc()call and avoids acquiring the stats mutex that could introduce latency to the critical PLC scan cycle.Reducing command logging verbosity - Removed all
log_debug("Received X command")statements for recognized commands (PING, STATUS, STOP, START, STATS, DEBUG) and the generic "Received command" log. Only unrecognized commands are now logged (via existinglog_error).Note: This is a breaking change for clients that expect
timing_statsin the status response by default. Per discussion, this is acceptable since the product hasn't been released yet.Review & Testing Checklist for Human
include_statsquery parameter is correctly passed to thedatadict by the REST API dispatch layer (checkrestapi.pyif needed)/api/statusreturns only{"status": "..."}without timing_stats when called without parameters/api/status?include_stats=truereturns both status and timing_statsRecommended test plan:
curl -k https://localhost:8443/api/statusreturns no timing_stats, whilecurl -k https://localhost:8443/api/status?include_stats=trueincludes timing_statsNotes
This is Stage 1 of a 3-stage fix. Stage 2 (openplc-web) and Stage 3 (openplc-editor) will update the clients to use
include_stats=trueonly when the orchestrators/device configuration screen is visible.Link to Devin run: https://app.devin.ai/sessions/23f46f5fc2d44440a67eda96a6048254
Requested by: Thiago Alves (@thiagoralves)