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

🥅 Makes ErrorEvent contains the optional exception stack trace #28

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

AlexV525
Copy link
Contributor

Description

The ErrorEvent for the relay client error channel does not contain enough info to extract where the exception is thrown. Adding the ErrorEvent.stackTrace will help the developer determine the root cause of the exceptions.

@quetool
Copy link
Member

quetool commented Oct 29, 2024

Hello @AlexV525! I will discuss this with the team as I'm quite sure we don't want to throw stack traces to the user/developer in these scenarios... These should be all user facing events so stack traces are meaningless here. If you could provide a useful scenario for doing this it would help me a lot!

@AlexV525
Copy link
Contributor Author

The reason is similar to #25, once you've swallowed the stack trace, developers will never know where goes wrong, especially when you report those errors to analysis platforms.

@quetool
Copy link
Member

quetool commented Oct 29, 2024

We can add logger methods for sure, like on line 217 or 222

core.logger.e('[$runtimeType]: Connect timeout: $e');
onRelayClientError.broadcast(ErrorEvent('Connection to relay timeout'));

But the main idea of onRelayClientError (or any other event subscription fwiw) is to allow the developer to subscribe to meaningful events and inform the user when they happen. Stack trace is not meaningful to the users.

I will add for sure logs in the next version but still kipping this PR opened to discuss it.
Thanks!

@AlexV525
Copy link
Contributor Author

But the main idea of onRelayClientError (or any other event subscription fwiw) is to allow the developer to subscribe to meaningful events and inform the user when they happen. Stack trace is not meaningful to the users.

What is the definition of "users" here? Is it "end users" or "developers"? For end users they surely don't need to care about anything with stack traces, but they are helpful for developers. One thing we've met in our app is that one of our stream listener was closed which cannot emmited further events but the channel event didn't provide the source. .stackTrace only implied in Error so when some other objects are thrown you will never know what is going on based on the current event structure.

@quetool
Copy link
Member

quetool commented Oct 29, 2024

What is the definition of "users" here? Is it "end users" or "developers"? For end users they surely don't need to care about anything with stack traces, but they are helpful for developers.

Exactly, users are end users. And for developers I will add a logging system there like in other places. If that is not helpful we can discuss this PR further/

@AlexV525
Copy link
Contributor Author

AlexV525 commented Oct 29, 2024

Logging requires extra effort for developers to track as they need to filter from logs and make connections between business and errors.
Considering you've added the logs, your project has several relay client implementations. How would you determine if an error occurred from RelayA rather than RelayB, and how do you manage error reporting?

EDIT: runtimeType is discouraged in practice. Logs are usually for CLI and server usage.

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.

2 participants