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

feat: Add configurable timeouts #320

Merged
merged 10 commits into from
Oct 3, 2024
Merged

Conversation

TheRealAgentK
Copy link
Contributor

@TheRealAgentK TheRealAgentK commented Sep 29, 2024

feat: #319 - Missing timeouts

Description 📝

  • Fixes Transport: Timeouts missing #319 and possibly improve or fix Question: any way to abort long-running send call? #311 (to be seen)
  • Adds timeout as an option and an explicit default HTTP timeout of 5000ms
  • Changes transport layer behaviour to destroy the request after various lifecycle events, in particular when the transport promise was completed successfully. This works around the default behaviour of http and https modules in Node to not stop a request even when it runs into a timeout
  • Unrelated change in the debug logs for breadcrumbs

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Updates

👉 Changed TS types
👉 Changes to transport layer
👉 Introduced new option
👉 Added tests to the mocking test bed
👉 TDB: Update documentation

Test plan 🧪

The easiest way to test under RL scenarios is with the express sample app. Set an API key and then configure raygun.client.js with options such as for example:

var raygunClient = new raygun.Client().init({
  apiKey: config.Raygun.Key,
  timeout: 10000,
  batch: true,
  batchFrequency: 10000
});

Author to check 👓

  • Project and all contained modules builds successfully
  • Self-/dev-tested
  • Unit/UI/Automation/Integration tests provided where applicable
  • Code is written to standards
  • Appropriate documentation written (code comments, internal docs)

Reviewer to check ✔️

  • Project and all contained modules builds successfully
  • Change has been dev-/reviewer-tested, where possible
  • Unit/UI/Automation/Integration tests provided where applicable
  • Code is written to standards
  • Appropriate documentation written (code comments, internal docs)

@TheRealAgentK TheRealAgentK changed the title feature: Add configurable timeouts feat: Add configurable timeouts Sep 29, 2024
Copy link
Contributor

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

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

Overall looks good! I think it can be simplified a bit if the RaygunOptions get unified to a single config property for timeout.

Also, we should document this option in the README.md before merging.

lib/types.ts Outdated Show resolved Hide resolved
lib/raygun.ts Outdated Show resolved Hide resolved
lib/raygun.transport.ts Outdated Show resolved Hide resolved
@TheRealAgentK
Copy link
Contributor Author

@miquelbeltran This is complete now and all PR comments resolved. Feel free to re-review.

Copy link
Contributor

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

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

LGTM!

@TheRealAgentK TheRealAgentK merged commit f656198 into develop Oct 3, 2024
6 checks passed
@sumitramanga sumitramanga deleted the feature/#319-timeouts branch December 3, 2024 03:12
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.

Transport: Timeouts missing Question: any way to abort long-running send call?
2 participants