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

Retry is disabled by default for POST method #1290

Open
mmiermans opened this issue Mar 4, 2024 · 2 comments
Open

Retry is disabled by default for POST method #1290

mmiermans opened this issue Mar 4, 2024 · 2 comments
Labels
type:defect Bugs or weaknesses. The issue has to contain steps to reproduce.

Comments

@mmiermans
Copy link

mmiermans commented Mar 4, 2024

Describe the bug
Retries are disabled by default for the POST method. We used a config similar to the one below, intending to have 3 retries:

gotEmitter(
  process.env.SNOWPLOW_ENDPOINT, // endpoint
  'https',
  undefined, // port
  'post', // method
  1, // buffer size
  3, // retries
);

However, Snowplow events were not retried on failure due to the Got defaults:

By default, Got does not retry on POST.

To Reproduce
Configure Snowplow with 'post' method and retries >= 1.

Expected behavior
If Snowplow is configured with a positive number of retries, then emit should be retried on network error.

Suggested solution
By default, enable retries for GET and POST methods in gotEmitter.

If the retry argument is a number (instead of an object), then the following object is passed to Got:

const retries: Partial<RequiredRetryOptions> = {
  limit: retry,
  methods: ['GET', 'POST'],
};

Otherwise, if the retry.methods is undefined it should default to be enabled for both methods used by Snowplow:

const retries: Partial<RequiredRetryOptions> = {
  methods: ['GET', 'POST'],
  ...retry,
};

Desktop (please complete the following information):

  • OS: Linux
  • Node: v20.11.0

Additional context
@snowplow/node-tracker version 3.19.0

@mmiermans mmiermans added the type:defect Bugs or weaknesses. The issue has to contain steps to reproduce. label Mar 4, 2024
@mmiermans
Copy link
Author

mmiermans commented Mar 4, 2024

Once we found out about this, a simple workaround is to set retry to:

{
  limit: 3,
  methods: ['GET', 'POST'],
};

However, many people (like us) might have simply passed in a number, and assumed that would work. We only found out about this once events seemed not to arrive, and we started investigating.

@matus-tomlein
Copy link
Contributor

Thanks for reporting this @mmiermans , that is a good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:defect Bugs or weaknesses. The issue has to contain steps to reproduce.
Projects
None yet
Development

No branches or pull requests

2 participants