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

Transport layer: Reduce complexity #321

Closed
TheRealAgentK opened this issue Sep 30, 2024 · 2 comments
Closed

Transport layer: Reduce complexity #321

TheRealAgentK opened this issue Sep 30, 2024 · 2 comments

Comments

@TheRealAgentK
Copy link
Contributor

TheRealAgentK commented Sep 30, 2024

Please describe your new feature request
The current solution for batch and non-batch transport is based on the setup of Promise that contains the message and an HTTP request wrapped into another Promise.

This mechanism works fine for non-batch transport as the Promise can then simply be resolved by using its own transport infrastructure.

For batch call that becomes more convoluted and essentially we're building up these messages with their own transport mechanism and stick them into (more or less) and array. Batching has its own, overarching HTTP request infrastructure though that gets set on the main raygun client class via transport().

That in itself is fine, but leads to waste and unnecessary complexity in the flow through the system. One effect is - it's actually quite hard to follow the flow, but more problematic is that we're building up this set of nested Promises with an HTTP request promise inside that the batch calls will never use and effectively just discard.

Describe the solution you'd like
This came out of #311 #319 and #320.

I think the current structure is overkill and should be simplified under the surface. There is no immediate need (and I might be wrong when I look closer into this) to tie the HTTP request so closely into the message object. They're essentially two separate things. The HTTP request can still be a promise but the classes can be structured in a way so that the flow is clean and only one HTTP transport system with one clear entry and exit point would exist.

Describe alternatives you've considered
The option is leaving it as it is at the cost of a too high complexity of the solution.

Additional context
This doesn't have to be fixed and changed immediately and it would only be an under-the-hood change. But I think the next time this area of the SDK is being worked on, changes should be made in this area.

@miquelbeltran
Copy link
Contributor

IMO, it depends if the user cares about the Raygun response or not.

If we could make it in a way that the send() method doesn't return the Raygun response, then the batch processing transport can be greatly simplified indeed.

A compromise could be that when batch transport is enabled, the send() method doesn't return the actual Raygun response but just null, as we do when the message gets stored (due to being offline). That way, the send() call doesn't block the user until the batch processing has run.

@TheRealAgentK
Copy link
Contributor Author

Closing in favour of #330

@github-project-automation github-project-automation bot moved this from Needs Triage to Merged in Raygun4Node board Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Merged
Development

No branches or pull requests

2 participants