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

Automated logging towards GRiSP.io #18

Merged
merged 22 commits into from
Apr 1, 2024
Merged

Automated logging towards GRiSP.io #18

merged 22 commits into from
Apr 1, 2024

Conversation

ziopio
Copy link
Member

@ziopio ziopio commented Jan 29, 2024

This logger is a copy from Adam's contribution in DAB.
The loop that gets the chunks and sends them via WS is handled with gen_statem timeouts in grisp_io_client.

If @bchassoul frontend changes https://github.com/stritzinger/grisp_manager/pull/41 are loaded into stage.grisp.io, it is possible to test this branch with the stage.
just set {domain, "stage.grisp.io"} in the grisp_io app config when deployng this app.
Your board needs to be linked to see its logs on the web page, but the logs will be sent anyway.

@ziopio
Copy link
Member Author

ziopio commented Mar 12, 2024

@ziopio ziopio force-pushed the ziopio/logger branch 4 times, most recently from 4042d76 to 965c1fd Compare March 15, 2024 10:44
@ziopio ziopio changed the title WIP Automated logging towards GRiSP.io Automated logging towards GRiSP.io Mar 15, 2024
@ziopio ziopio requested a review from maehjam March 15, 2024 11:06
src/grisp_io_client.erl Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/grisp_io.app.src Show resolved Hide resolved
src/grisp_io_binlog.erl Show resolved Hide resolved
src/grisp_io_client.erl Outdated Show resolved Hide resolved
src/grisp_io_client.erl Outdated Show resolved Hide resolved
src/grisp_io_client.erl Outdated Show resolved Hide resolved
src/grisp_io_logger_bin.erl Show resolved Hide resolved
src/grisp_io_client.erl Outdated Show resolved Hide resolved
@bchassoul bchassoul self-requested a review March 16, 2024 01:01
src/grisp_io_sup.erl Outdated Show resolved Hide resolved
Maximum size is hardcoded to 30_000 to avoid rtems bug
Truncate function is not dropping the target index returned by the server
Maximum size is hardcoded to avoid rtems bug
@@ -80,7 +80,7 @@ truncate(To, #binlog{seq = Seq} = L) when To >= Seq ->
clear(L);
truncate(To, #binlog{queue = Q0} = L) ->
case queue:out(Q0) of
{{value, {Seq, Bin}}, Q1} when Seq < To ->
{{value, {Seq, Bin}}, Q1} when Seq =< To ->
Copy link
Member Author

@ziopio ziopio Mar 22, 2024

Choose a reason for hiding this comment

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

This bug could also affect DAB
It happens when the client does not send the whole log queue and later syncs the logs.
This can result in a repeated log sent to the server or, way worse, infinite loop.
I had an infinite loop in my case because now I am forcing the chunk to be under a fixed byte size. So I had a queue long 2 and always sending only one log. This was resulting in a failed sync because of this truncate function. And this would endup looping forever.

Copy link
Member Author

@ziopio ziopio Mar 22, 2024

Choose a reason for hiding this comment

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

For DAB this may cause a log line to be sent 2 times.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used this image as reference for the protocol algorithm https://github.com/stritzinger/dab/blob/master/docs/log_buffer.excalidraw.png

@ziopio ziopio requested a review from maehjam March 22, 2024 15:18
README.md Outdated
Once this app is started, it forwards all logs to GRiSP.io without the need of setting up anything. The only logs that we do not catch are the ones generated before `grisp_io` boots.
If you want to see ALL logs, even from applications that boot before `grisp_io`, you need to disable the default logger and set our logger as the default one. This involves changing the `kernel` and `grisp_io` app configuration settings in your sys.config file.

You can copy paste these settings. Here we both swap the default logger with ours and also request our logger to print logs to stdout.
Copy link
Member

Choose a reason for hiding this comment

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

Wording is a bit strange. It's not our logger, but a handler for the OTP logger. I would rather talk about "the grisp_io logger handler" (or when we rename the repo "the grisp_connect logger handler"). Same for default logger, it's the default logger handler.

@ziopio ziopio requested review from GwendalLaurent and removed request for bchassoul March 22, 2024 15:32
Copy link
Member

@maehjam maehjam left a comment

Choose a reason for hiding this comment

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

This PR looks big in number of lines, but it's only one feature. IMO we can squash and merge it and don't need to split it up in several commits.

@bchassoul bchassoul merged commit bb869dc into main Apr 1, 2024
1 check passed
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.

4 participants