Pull Logging and Logic Improvements #127

BLuedtke · 2023-11-12T09:12:44Z

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:

Parse form data
Validate form data, Log ERR if validation fails
Validation succeeded -> Log request start
Processing request
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.

Fix log type keywords

BLuedtke · 2023-11-16T07:39:06Z

This concludes the changes I wanted to make before testing.

BLuedtke · 2023-11-16T15:37:15Z

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 · 2023-11-16T16:57:39Z

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.

eyip002

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

server/src/handler_driver.c

server/src/handler_monitor.c

server/src/handler_upload.c

BLuedtke · 2023-11-28T12:13:32Z

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 · 2023-11-28T12:39:07Z

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 · 2023-11-28T12:47:01Z

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).

eyip002

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

README.md

server/src/handler_upload.h

server/src/websocket_uploader/engine_uploader.c

BLuedtke · 2024-01-17T13:52:58Z

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 · 2024-01-17T14:07:13Z

#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.

BLuedtke added 10 commits February 21, 2023 17:01

added entry/finished logs for all onion handler callbacks

8753e7e

further fixes

747627d

Improve log schema consistency

1b48b76

Fix log type keywords

Improvements and Merge from branch master

2c7b38c

Whitespace and formatting

d5ee500

Request Logging Descript in Readme and verif new log

542c2d8

improve logging, validation, logic in admin, controller

93fefaa

improve logging, logic, in driver and monitor

afe862d

improve logging in upload

9f72567

outfactor common grab-id and train-id check

88b85b6

BLuedtke self-assigned this Nov 12, 2023

BLuedtke added 14 commits November 12, 2023 10:42

Improve includes (order, duplicates)

a8799cb

improved logging when stopping server

39c1783

improved startup logging

4d6f1e2

admin handler minor log improv

ad30790

consistent log before or after mutex in driveroute

2c76bf3

admin import removed dupl and unused

94a0037

Applied (multi-arg) formatting scheme

0005f7d

removed unnecessary log in upload dupl check

2eb3304

merged some lines that stay < 100

8e2d947

moved train time step constant to header to resolve multi-definition

d87853e

Add missing va_end

e91ebe4

Updated Readme

ed042ad

improve name and log consistency for admin handler

e8da5f6

further name and log consistency improvements

dc42190

BLuedtke added 2 commits November 16, 2023 17:16

Fix issues mentioned in PR

c911869

rephrase misleading log in dyn containers interface

4dc17b9

remove unnecessary route pointer lookup

81e3e5b

eyip002 requested changes Nov 24, 2023

View reviewed changes

BLuedtke and others added 5 commits November 25, 2023 10:53

readme adjustments after review

ddc4882

part of review comments addressed

72629b3

readme review comments addressed

c425513

review comment addressing up to driver release train

2ad2095

handler driver fixes & improvements

8f585b5

BLuedtke added 2 commits November 28, 2023 13:29

add "start" to req handler log pattern

09cd8c3

move "getter" handlers to log only "done"

9d54f71

add apt update to pipeline steps

ebb0e4d

BLuedtke added 9 commits November 28, 2023 13:50

readme log pattern refined

9928d8b

refactored verification option string parser

029beee

engine uploader basic log refactor

8a1088a

engine uploader context for WS related logs

463636c

engine uploader formatting

042ec48

admin verification URL formatting

b1c70ce

updated handler log format with abort and finish

fe82c6c

address "granting in progress" discussion

021c458

remove hardcoding route conflicts array length

c6db384

eyip002 requested changes Dec 1, 2023

View reviewed changes

Update Readme

c616f7f

log consistency in engine uploader

3975382

eyip002 approved these changes Jan 22, 2024

View reviewed changes

eyip002 merged commit 2b593b0 into master Jan 22, 2024
1 check passed

eyip002 deleted the improve-logging branch January 22, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull Logging and Logic Improvements #127

Pull Logging and Logic Improvements #127

Pull Logging and Logic Improvements #127

Pull Logging and Logic Improvements #127

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment