fix: improve logging

[ericvergnaud]

No description provided.

[codecov-commenter]

Codecov Report

Merging #1573 (d5c4263) into main (c0d9304) will increase coverage by 0.00%.
Report is 3 commits behind head on main.
The diff coverage is 94.73%.

@@           Coverage Diff           @@
##             main    #1573   +/-   ##
=======================================
  Coverage   85.73%   85.73%           
=======================================
  Files         950      950           
  Lines       22745    22761   +16     
  Branches     3978     3982    +4     
=======================================
+ Hits        19500    19514   +14     
- Misses       3060     3062    +2     
  Partials      185      185           

Files Changed	Coverage Δ
...ore/src/modules/message-pìckup/MessagePickupApi.ts	82.85% <83.33%> (-1.02%)	⬇️
packages/core/src/agent/MessageSender.ts	88.10% <100.00%> (+0.05%)	⬆️
packages/core/src/agent/TransportService.ts	96.55% <100.00%> (+1.31%)	⬆️
packages/core/tests/logger.ts	86.04% <100.00%> (ø)

... and 1 file with indirect coverage changes

[jakubkoci]

Hi @ericvergnaud . Thanks for those log additions, to me, it's always hard to evaluate right place to log and not overlog. :)

[ericvergnaud]

Hi,
I see that some tests are failing but I very much doubt that has anything to do with the log messages...
Is there anything I need to do ?

[ericvergnaud]

@jakubkoci hey, any chance we can finalize this ? Looks like 'main' is evolving rapidly making it more difficult to merge every day ?

[genaris]

Thanks @ericvergnaud ! I think it is more than just an improvement of logging, as it now handles a possible error when sending a message through a WebSocket. In regards to that I'd like to make sure that the method won't return unless it is sure it could write on the socket. Maybe it is already doing so, but there are also other possibilities (like using rxjs magic as we do in other parts of the framework) to wait for the callback execution and return.

[ericvergnaud]

@genaris re #1573 (comment) yes the callback is only called after send completes, see https://github.com/websockets/ws/blob/master/doc/ws.md#websocketsenddata-options-callback

[genaris]

@genaris re #1573 (comment) yes the callback is only called after send completes, see https://github.com/websockets/ws/blob/master/doc/ws.md#websocketsenddata-options-callback

It is clear to me that the callback is called after socket is written, but my question is if this procedure is done synchronously: will the function will block until it gets a successful or failed status on the write operation?

ws.send() is calling socket.write() under the hood and simply passing the provided callback. In its documentation it states The optional callback parameter will be executed when the data is finally written out, which may not be immediately. Also it says that its (boolean) return value depends on the data flushed to kernel buffer.

So I'm wondering if socket.write() (and therefore ws.send()) will return immediately and executing the callback asynchronously. If that's the case, it might happen that the application continues the execution after send() and the error is thrown some time later, which maybe is not handled as we would like?

Anyway, apart from this curiosity, I still see the unrelated changes in this PR. You'll need to rebase/merge your branch proerly, otherwise it will not be possible to merge it into main once approved.

[ericvergnaud]

In my understanding and experience, ws.send()is synchronous, even if it's not immediate, and the callback is executed in the context of the caller, so the exception bubbles up properly.

[ericvergnaud]

I'll try rebasing

[ericvergnaud]

Rebasing done

[genaris]

In my understanding and experience, ws.send()is synchronous, even if it's not immediate, and the callback is executed in the context of the caller, so the exception bubbles up properly.

As long as the error is thrown on the same context of the caller, I'm OK with it.