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

Refactor the JSON-RPC connection #52

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
17 changes: 17 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,23 @@ and this project adheres to

## [Unreleased]

### Changed

- The name of the grisp_connect configuration key to control the timeout of
individual JSON-RPC requests changed from ws_requests_timeout to
ws_request_timeout.
- Le default log filter changed to trying to filter out only some messages to
Copy link
Member

Choose a reason for hiding this comment

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

"Le" -> "The".
Maybe mention that this is only done for testing and development. (I see log level in prod is notice so we don't need it there.)

filtering out all progress messages, as it wasn't working reliably.
- JSON-RPC logic was extracted into the jarl library.
- Jarl parses the methods into a list of atom or binaries to pave the
road for namespaces. foo.bar.Buz is parsed into [foo, bar, <<"Buz">>] (if foo
and bar are already existing atoms, but 'Buz' is not).

## Fixed

- The client is now waiting 1 second before trying to reconnect when it gets
disconnected from the server.
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to find this. Is this done here or in jarl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is done in in grisp_connect_client, in the connecting function.

Copy link
Member

Choose a reason for hiding this comment

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

I did look again and found it in waiting_ip. So that is a counter intuitive place to do it. I was expecting it in connecting somewhere, for example in enter there. Is there some deeper reason to put it in waiting_ip that I don't see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make much sense to not check again if the IP is available after a connection error, so going back to waiting_ip seems better, and as we want to wait between each ip checks, better leveraging this than behaving differently if we were connected or not before then.

Copy link
Member

Choose a reason for hiding this comment

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

But it's not leveraged. The waiting between waiting_ip is implemented in line 156 and the waiting between connects in line 144. They are not related at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, because by switching to state waiting_ip from either the connecting or connected state you will have the correct delay. Maybe using repeat_state instead of next_state at line 156 and not registering a new state_timeout would make it clearer for you ? This way the timeout would only be setup in waiting_ip enter function.


## [1.1.0] - 2024-10-12

### Added
Expand Down
10 changes: 4 additions & 6 deletions config/dev.config
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,16 @@
]},

{kernel, [
{logger_level, info},
{logger_level, debug},
{logger, [
{handler, default, logger_std_h, #{
config => #{type => standard_io},
filter_default => log,
filters => [
% Filter out supervisor progress reports so
% TLS certificates are not swamping the console...
{filter_out_progress, {
fun grisp_connect_logger_bin:filter_out/2,
{supervisor, report_progress}}}
{disable_progress, {fun logger_filters:progress/2, stop}}
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to keep the comment why we do that here and also add it to local.config, test.config.

]
}}
]}
]}

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary newline.

].
13 changes: 12 additions & 1 deletion config/local.config
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,16 @@
{port, 8443}
]},

{kernel, [{logger_level, debug}]}
{kernel, [
{logger_level, debug},
{logger, [
{handler, default, logger_std_h, #{
config => #{type => standard_io},
filter_default => log,
filters => [
{disable_progress, {fun logger_filters:progress/2, stop}}
]
}}
]}
]}
].
13 changes: 13 additions & 0 deletions config/test.config
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,18 @@
{grisp_connect, [
{domain, localhost},
{port, 3030}
]},

{kernel, [
{logger_level, debug},
{logger, [
{handler, default, logger_std_h, #{
config => #{type => standard_io},
filter_default => log,
filters => [
{disable_progress, {fun logger_filters:progress/2, stop}}
]
}}
]}
]}
].
69 changes: 69 additions & 0 deletions docs/grisp_connect_architecture.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# Architecture

```mermaid
graph TD
RootSup[VM Root Supervisor]
subgraph GrispConnectApp[Grisp Connect Application]
GrispConnectRootSup[Root Supervisor<br>grisp_connect_sup]
GrispConnectLogServer[Log Server<br>grisp_connect_log_server]
GrispConnectClient[Client<br>grisp_connect_client]
GrispConnectRootSup --Supervise--> GrispConnectLogServer
GrispConnectRootSup --Supervise--> GrispConnectClient
GrispConnectClient --Spawn and Monitor--> JarlConnection
end
subgraph JarlApp[Jarl Application]
JarlConnection[JSON-RPC Connection<br>jarl_connection]
JarlJsonRpc[JSON-RPC Codec<br>jarl_jsonrpc]
JarlConnection --Use--> JarlJsonRpc
end
subgraph GunApp[Gun Application]
GunRootSup[Gun Root Supervisor<br>gun_sup]
GunConnsSup[Gun Connection Supervisor<br>gun_conns_sup]
Gun[Gun Connection<br>gun]
Gun[Gun HTTP Handler<br>gun_http]
GunRootSup --Supervise--> GunConnsSup
GunConnsSup --Supervise--> Gun
end
RootSup --Supervise--> GrispConnectRootSup
RootSup --Supervise--> GunRootSup
JarlConnection -.Interact.-> Gun
```


## Client

The client process is the main state machine. Its responsabilities are:

- Trigger connection/reconnection to the backend.
- Expose high-level protocol API to the application.
- Implement generic API endpoints.

See the [client documentation](grisp_connect_client.md).


## Connection

grisp_connect use the jarl library to handle the JSON-RPC connection.

It is not supervised, the process starting it must monitor it.

It provides a high-level API to a JSON-RPC connection:

- Perform synchronous requests
- Start asynchronous requests
- Reply to a request
- Send and error result for a request
Copy link
Member

Choose a reason for hiding this comment

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

"and" -> "an"

- Send asynchronous notifications
- Send generic errors
Copy link
Member

Choose a reason for hiding this comment

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

That formulation is a bit unclear. Do you mean the automatic sending for predefined JSON-RPC errors from the JSON-RPC specifications? Then instead of "generic errors" rephrase it to "predefined JSON-PRC errors".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something we figured out wasn't even in the JSON-RPC spec, and I removed it. O I will remove the generic error line.


When performing an asynchronous request, the caller can give an opaque context
term, that will given back when receiving a response or an error for this
Copy link
Member

Choose a reason for hiding this comment

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

"that will be given back"

request, allowing the caller to handle the asynchronous operation without having
to store information locally.
4 changes: 2 additions & 2 deletions rebar.config
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{erl_opts, [debug_info]}.
{deps, [
{grisp, "~> 2.7"},
{gun, "2.1.0"},
jsx,
{jarl, {git, "https://github.com/grisp/jarl.git"}},
{grisp, "~> 2.7"},
{grisp_cryptoauth, "~> 2.4"},
{certifi, "2.13.0"}
]}.
Expand Down
34 changes: 16 additions & 18 deletions src/grisp_connect.app.src
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@
kernel,
inets,
stdlib,
jsx,
jarl,
grisp,
grisp_cryptoauth,
gun,
jsx,
certifi
]},
{optional_applications, [
Expand All @@ -21,26 +21,24 @@
{port, 443},
{connect, true}, % keeps a constant connection with grisp.io
{ntp, false}, % if set to true, starts the NTP client
{ws_requests_timeout, 5_000},
{ws_transport, tls},
{ws_path, "/grisp-connect/ws"},
{ws_request_timeout, 5_000},
{ws_ping_timeout, 60_000},
{logs_interval, 2_000},
{logs_batch_size, 100},
{logger, [
% Enable our own default handler,
% which will receive all events from boot
{handler,
grisp_connect_log_handler,
grisp_connect_logger_bin,
#{formatter => {grisp_connect_logger_bin, #{}},
% Filter out supervisor progress reports to prevent the ones
% from tls_dyn_connection_sup that logs all the certificates
% to crash the connection...
filters => [
{filter_out_progress,
{fun grisp_connect_logger_bin:filter_out/2,
{supervisor, report_progress}}}
]
}}
% Enable our own default handler,
% which will receive all events from boot
{handler,
grisp_connect_log_handler,
grisp_connect_logger_bin,
#{formatter => {grisp_connect_logger_bin, #{}},
filter_default => log,
filters => [
{disable_progress, {fun logger_filters:progress/2, stop}}
Copy link
Member

Choose a reason for hiding this comment

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

Note that If this should ship in the default dettings for a grisp_connect based project we need to open a PR for rebar3_grisp/ grisp_tools to update the configuration for new generated user projects;
(rebar3 grisp configure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not 100% sure that it make things better. It looked better for me, and this is the common solution I found about our problem with logging certificate, but others should test it too.

]
}}
]}
]},
{modules, []},
Expand Down
Loading
Loading