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

[exporter/prometheusremotewrite] Add the possibility to disable queuing #6718

Merged
merged 2 commits into from
Dec 21, 2021

Conversation

trasc
Copy link
Contributor

@trasc trasc commented Dec 13, 2021

Description: Add the possibility to disable the sending queue in prometheusremotewrite exporter

A new configuration key, remote_write queue.enabled is added, which
controls the queue enabling.

Link to tracking Issue: N/A

Testing: No regressions on local runes

Documentation: Small update of README.md

@trasc trasc requested review from a team and mx-psi December 13, 2021 09:08
@trasc trasc force-pushed the prw_async branch 2 times, most recently from 8a16fa2 to 57bb1a0 Compare December 15, 2021 06:29
@mx-psi
Copy link
Member

mx-psi commented Dec 15, 2021

@trasc What's the current behavior if you set the queue size to zero? Does the exporter work at all?

@trasc
Copy link
Contributor Author

trasc commented Dec 15, 2021

@trasc What's the current behavior if you set the queue size to zero? Does the exporter work at all?

Currently, when set to 0 , the queue is always "full", and just drops any incoming data.

@mx-psi mx-psi requested a review from rakyll December 15, 2021 12:03
@mx-psi
Copy link
Member

mx-psi commented Dec 15, 2021

I see, thanks! I think there are two issues here:

  1. the exporterhelper should not allow setting the queue size to zero (maybe with a Validate helper?) and
  2. people may want to disable the queue entirely on this exporter in particular.

For (1), can you open an issue on github.com/open-telemetry/opentelemetry-collector?

For (2), I feel like it's more consistent to add a new enabled field for this instead of reusing the queue_sizefor enabling/disabling this. What do you think? Would also want @rakyll to confirm that it's safe to disable the queue.

@jpkrohling jpkrohling changed the title [prometheusremotewriteexporter] Disable queuing when queue_size is 0 [exporter/prometheusremotewrite] Disable queuing when queue_size is 0 Dec 16, 2021
@trasc
Copy link
Contributor Author

trasc commented Dec 16, 2021

I see, thanks! I think there are two issues here:

  1. the exporterhelper should not allow setting the queue size to zero (maybe with a Validate helper?) and

For (1), can you open an issue on github.com/open-telemetry/opentelemetry-collector?

I don't think that adding Validate will be very effective, since is very likely to never be called unless the user will specifically do it. In this case QueueSettings is built only on component creation not on config validation. The best way that I could think of to enforce this kind of config constraints will be to do it when applying the Options, but unfortunately, the Option function prototype is not returning an error. Changing the Option prototype just for the exporterhelper will brake the Options pattern across the project.

Another approach could be just to make sure that the scenario is properly documented in https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/README.md,

  1. people may want to disable the queue entirely on this exporter in particular.

For (2), I feel like it's more consistent to add a new enabled field for this instead of reusing the queue_sizefor enabling/disabling this. What do you think? Would also want @rakyll to confirm that it's safe to disable the queue.

I've added a dedicated config key enabled and a new error indicating that a 0 sized queue will result in dropping all data.

@trasc trasc changed the title [exporter/prometheusremotewrite] Disable queuing when queue_size is 0 [exporter/prometheusremotewrite] Add the possibility to disable queuing Dec 16, 2021
Traian Schiau added 2 commits December 16, 2021 14:19
A new configuration key, `remote_write queue.enabled` is added, which
controls the queue enabling.
Update the expected prometheusremotewriteexporter default config.
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Changes on this exporter look good. Will create an issue to further discuss how to deal with the 0 queue size at the exporterhelper level.

@anuraaga If you find the time, would you mind reviewing this to get it merged before the holidays? 🙏

@jpkrohling
Copy link
Member

@anuraaga, if you are OK with this change, ping me and I'll merge it today. Otherwise, it will probably have to wait until next year :-)

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Apologies have been having trouble following my notification lately

@jpkrohling jpkrohling merged commit 270b2c4 into open-telemetry:main Dec 21, 2021
povilasv referenced this pull request in coralogix/opentelemetry-collector-contrib Dec 19, 2022
…ettings (#6718)

* Remove deprecated comonent.Config.[ID|SetIDName]; Deprecate config.*Settings

Signed-off-by: Bogdan Drutu <[email protected]>

* Update .chloggen/rmcfgid-1.yaml

Co-authored-by: Pablo Baeyens <[email protected]>

Signed-off-by: Bogdan Drutu <[email protected]>
Co-authored-by: Pablo Baeyens <[email protected]>
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.

5 participants