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

Add new parameter to limit size of the internal queue by log events count #315

Closed
lucasoares opened this issue May 9, 2023 · 4 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@lucasoares
Copy link

Since #203 the limit is defined in bytes.

Seq's sink have the property to limit the queue size by number of log events:

    /// <param name="queueSizeLimit">The maximum number of events that will be held in-memory while waiting to ship them to
    /// Seq. Beyond this limit, events will be dropped. The default is 100,000. Has no effect on
    /// durable log shipping.</param>

I'm migrating to Http sink but I would like to keep the same configurations. Can we to have those two properties existing at the same time in the Http sink?

Thanks.

@lucasoares lucasoares added the enhancement New feature or request label May 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

Hi there and welcome to this repository!

A maintainer will be with you shortly, but first and foremost I would like to thank you for taking the time to report this issue. Quality is of the highest priority for us, and we would never release anything with known defects. We aim to do our best but unfortunately you are here because you encountered something we didn't expect. Lets see if we can figure out what went wrong and provide a remedy for it.

@FantasticFiasco
Copy link
Owner

Hi @lucasoares!

The change from specifying queue size based on total event sizes instead of number of events was a deliberate decision. It's now way easier to reason about resource requirements.

I don't think re-adding the configuration would be beneficial. Please consider to specify a queue size instead of specifying maximum number of events.

@lucasoares
Copy link
Author

Hello!

I understand this is a deliberate decision from the past, but why would it not be beneficial? If other sinks provide this configuration, this could allow an easier as-is migration. I want to migrate from the Seq sink but fewer changes in my system = better.

We could even improve the performance to avoid incrementing when this configuration is disabled (no limit) to avoid allocations (which I don't think is needed since it is a simple integer). Still, I can't see why this sink should not support this limit using the count of events. Working with both parameters together is very beneficial in this case.

@FantasticFiasco
Copy link
Owner

It's hard for me to argue that this feature wouldn't provide value, as you say yourself you would benefit from having this property. But as a maintainer of a project I have to reflect on the cost as well. With reintroducing this property we add implementation complexity, this leads to more tests. From an API perspective this is also worse, the intuitiveness has decreased, two parameters are used to basically control the same thing.

There was a great article released just a few days ago talking about open source from the perspective of the maintainer. I think you should read it, it gives you a sense of what maintainers have to think about in relation to contributors.

Regarding your thought on performance, these are theoretical performance gains, I don't see you pointing me to any actual benchmarks. I think that these optimizations would be negligible compared to the time we spend sending data over the network, an operation that more than anything defines the performance of this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants