Skip to content
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.

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

Pull Logging and Logic Improvements #127

Merged
merged 46 commits into from
Jan 22, 2024
Merged

Pull Logging and Logic Improvements #127

merged 46 commits into from
Jan 22, 2024

Conversation

BLuedtke
Copy link
Collaborator

Alright, the "Improve Logging" branch has grown to incorporate a bit more than just logging improvements.

  • I added some Validation improvements outside of request handlers.
  • Some formatting fixes

The biggest changes are in the request handlers. I moved the validation around such that they follow a pattern:

  1. Parse form data
  2. Validate form data, Log ERR if validation fails
  3. Validation succeeded -> Log request start
  4. Processing request
  5. Log that processing has finished (or failed, respectively).

The "log request start" is also outfitted with data about request parameters, such that one can trace after the fact what request is which, if there are multiple trains/users active. Furthermore, the "log request finished" is the exact same as the "log request start" if possible, just with a "finished" at the end. This makes it relatively easy to trace when a request came in and when it was finished.

Since I already had to touch most of the handlers and some related functions, I made some adjustments to them, mostly regarding validation and formatting. There are also some notes I made on areas of the codebase that may need some change but where we should really discuss it first.
This is a bit of a bigger change, but IMO prepares us to tackle the API rework.

@BLuedtke BLuedtke self-assigned this Nov 12, 2023
@BLuedtke
Copy link
Collaborator Author

This concludes the changes I wanted to make before testing.

@BLuedtke
Copy link
Collaborator Author

BLuedtke commented Nov 16, 2023

Known mistakes:

  • driver release train: sometimes printed with "train", sometimes with "train id"
  • request route: "from" parameter in "finished" log is the route id, but should be the source signal.
  • wrong log when setting interlocker leads to segmentation fault

@BLuedtke
Copy link
Collaborator Author

BLuedtke commented Nov 16, 2023

Tested the correctness of the logs for all endpoints except for monitor/pure getters.
log2.txt
log2-clientside.txt

For pure getters, i.e. those handlers that only log on the level LOG_NOTICE, I tested that they respond as expected, but I did not check the correctness of their logs (not enough time).

The previous commit resulted from this testing.

Copy link
Member

@eyip002 eyip002 left a comment

Choose a reason for hiding this comment

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

For human-readability, I think it is necessary to add - start because it's very hard to remember the message format to look for until you reach the message with - finish (then you can backtrack to the corresponding "start" message.


Should be tested with the web clients to make sure they can still parse the server responses.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
server/src/handler_driver.c Outdated Show resolved Hide resolved
server/src/handler_monitor.c Outdated Show resolved Hide resolved
server/src/handler_monitor.c Show resolved Hide resolved
server/src/handler_upload.c Outdated Show resolved Hide resolved
server/src/handler_upload.c Outdated Show resolved Hide resolved
@BLuedtke
Copy link
Collaborator Author

BLuedtke commented Nov 28, 2023

For human-readability, I think it is necessary to add - start because it's very hard to remember the message format to look for until you reach the message with - finish (then you can backtrack to the corresponding "start" message.

I agree. Also: Should it be "finish" instead of "finished"?

Somewhat related: Should the error messages after - start, that also indicate end of processing, have something like - abort? It'd clarify that it's the end of processing at the cost of (probably) more line breaks. On the other hand, an error log at the end of processing is not really "aborting", so not sure if that term is suitable.

@BLuedtke
Copy link
Collaborator Author

The ci is failing due to some build setup issue, specifically when trying to install libgnutls. Will also have to look at that at some point.

@BLuedtke
Copy link
Collaborator Author

Pipeline works again, added apt update before any step where apt packages are installed.
In the long term (regarding the ci pipeline), we may also want to switch from ubuntu-latest to a fixed ubuntu version (currently ubuntu-latest seems to be Jammy/22.04).

Copy link
Member

@eyip002 eyip002 left a comment

Choose a reason for hiding this comment

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

For the code documentation, think about whether libbidib's doxygen setup could be reused.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
server/src/handler_upload.h Show resolved Hide resolved
server/src/websocket_uploader/engine_uploader.c Outdated Show resolved Hide resolved
server/src/websocket_uploader/engine_uploader.c Outdated Show resolved Hide resolved
@BLuedtke
Copy link
Collaborator Author

For the code documentation, think about whether libbidib's doxygen setup could be reused.

I'd cover that in a future issue/pr, not in this one

@BLuedtke
Copy link
Collaborator Author

BLuedtke commented Jan 17, 2024

#127 (comment) somehow I cannot answer to that comment/change request. Not sure why. I'm adjusting the message in a way similar to the proposed message.

@eyip002 eyip002 merged commit 2b593b0 into master Jan 22, 2024
1 check passed
@eyip002 eyip002 deleted the improve-logging branch January 22, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants