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

Revise the emitter behaviour #850

Merged
merged 3 commits into from
Dec 8, 2023
Merged

Conversation

matus-tomlein
Copy link
Contributor

@matus-tomlein matus-tomlein commented Dec 1, 2023

This PR contains three changes:

Flush events only when the buffer is full (#827, contributed by @danigutierrezayuso)

Previously the buffer size configuration didn't work as intended – requests to the collector were initiated right after the event was tracked regardless of the buffer size.

This change fixes that and only makes the request once the buffer size is reached.

Change default buffer option to single (#849)

As mentioned before, previously the buffer size was effectively equal to 1 (even though we stated it was 10). Fixing the above bug, we would now change the buffer size to 10.

Buffer size 1 is probably a better default for client side trackers to avoid losing events that don't make it to a request before an app is quit/uninstalled. Also the same is used on the JavaScript tracker.

This change sets the default buffer size to 1 (from a user's point of view there won't be a difference compared to previous versions).

Make network requests serially in network connection (#827)

This change tries to address a problem with duplicate events often showing up in the warehouse. It is possible that this is called by sending too many parallel requests at once which causes either the requests to fail or timeout and then be retried.

After doing some tests, I discovered that making parallel requests from the client has very little benefit. The total time it takes to send all the events is very close to what it would be if requests were made serially.

The new behaviour updates how the emitRange configuration is used – it now tells how many events should be added to one request. For this reason, I also lowered the default value to 25.

When merging this PR, I will rebase the commits so that they show up individually in the release branch.

@matus-tomlein matus-tomlein force-pushed the issue/820-improve_concurrency_modeel branch from 1e97006 to 1c8b9cc Compare December 1, 2023 14:43
Base automatically changed from issue/820-improve_concurrency_modeel to release/6.0.0 December 1, 2023 15:17
@matus-tomlein matus-tomlein marked this pull request as ready for review December 1, 2023 15:20
@matus-tomlein matus-tomlein force-pushed the release/6.0.0 branch 2 times, most recently from 3c9165b to 22e2079 Compare December 7, 2023 12:23
Copy link
Contributor

@mscwilson mscwilson left a comment

Choose a reason for hiding this comment

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

LGTM!

@matus-tomlein matus-tomlein merged commit ddd6ca3 into release/6.0.0 Dec 8, 2023
20 checks passed
@matus-tomlein matus-tomlein deleted the issue/827-buffering branch December 8, 2023 14:57
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.

3 participants