-
Notifications
You must be signed in to change notification settings - Fork 26
fix(js-client): Improve logging #418
Conversation
| log.trace( | ||
| "sending the particle %s to the created stream %s", | ||
| particle.id, | ||
| stream.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better print relayAddress here? stream.id is a very low-level thing, afaik.
| log.trace("data written to sink"); | ||
|
|
||
| log.trace( | ||
| "the particle %s have been written to the stream %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "the particle %s have been written to the stream %s", | |
| "particle %s sent to %s", |
and relay address here, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I think that's debug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, stream.id seems redundant.
Not sure about relay address though, JS client always has a single relay address and it would spam the logs without particular benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imagine you have several js clients
|
|
||
| log_particle.trace( | ||
| log_particle.debug( | ||
| "id %s. executing call service handler %j", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe show here serviceId and functionName?
req seems to have way too much info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
req also contains particle context and arguments for the service call which could be important sometimes.
I tried to split the line above on 2 different logs - which does just what you say and more detailed with trace log level. Please check
folex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a log for incoming particle? Would be nice to see something like incoming particle {particle.id} from {relay.peerId}
|
|
||
| const sink = stream.sink; | ||
|
|
||
| await pipe([fromString(serializeToString(particle))], encode, sink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there never an error here? 🤔 Or is there a log for particle sending error in a separate place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, particle log for network error resides in calling class - FluencePeer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
No description provided.