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.

Sign up for GitHub

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

Jump to bottom

Allow users to configure the Prometheus remote write queue #3046

Merged
merged 9 commits into from
May 17, 2021
Merged

Allow users to configure the Prometheus remote write queue #3046

merged 9 commits into from
May 17, 2021

Conversation

rakyll
Copy link
Contributor

@rakyll rakyll commented Apr 28, 2021 •
edited
Loading

A follow up to #2974.

@codecov
Copy link

codecov bot commented Apr 28, 2021 •
edited
Loading

Codecov Report

Merging #3046 (00bf445) into main (73b55f0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 00bf445 differs from pull request most recent head 3ab55aa. Consider uploading reports for the commit 3ab55aa to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3046   +/-   ##
=======================================
  Coverage   92.06%   92.07%           
=======================================
  Files         313      313           
  Lines       15439    15449   +10     
=======================================
+ Hits        14214    14224   +10     
  Misses        817      817           
  Partials      408      408
Impacted Files Coverage Δ
exporter/prometheusremotewriteexporter/config.go 100.00% <ø> (ø)
exporter/prometheusremotewriteexporter/exporter.go 90.54% <100.00%> (ø)
exporter/prometheusremotewriteexporter/factory.go 100.00% <100.00%> (ø)
component/application_info.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e28b1ca...3ab55aa. Read the comment docs.

@rakyll rakyll marked this pull request as ready for review April 28, 2021 22:06
@rakyll rakyll requested a review from a team April 28, 2021 22:06
@rakyll
Copy link
Contributor Author

rakyll commented May 5, 2021

cc @odeke-em @Aneurysm9 @dashpole

@rakyll rakyll requested a review from dashpole May 5, 2021 20:53
odeke-em
odeke-em approved these changes May 5, 2021
View reviewed changes
Copy link
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @rakyll!

exporter/prometheusremotewriteexporter/exporter.go Outdated Show resolved Hide resolved
exporter/prometheusremotewriteexporter/factory.go Show resolved Hide resolved
@rakyll
Copy link
Contributor Author

rakyll commented May 6, 2021

cc @alolita to label this PR.

@alolita
Copy link
Member

alolita commented May 6, 2021

@anuraaga @open-telemetry/collector-approvers can you please review so that we can merge this fix. We are blocked on this fix for other dependent prometheus issues.

@alolita
Copy link
Member

alolita commented May 6, 2021

@jrcamp @jpkrohling - can you please approve asap.

@alolita alolita added area:prometheus ready-to-merge Code review completed; ready to merge by maintainers labels May 7, 2021
@jpkrohling
Copy link
Member

jpkrohling commented May 10, 2021 •
edited
Loading

I'll let someone with knowledge of the metrics side of things review this one.

@rakyll
Copy link
Contributor Author

rakyll commented May 11, 2021

@jpkrohling can you assign someone?

@rakyll
Copy link
Contributor Author

rakyll commented May 15, 2021

Can someone merge this PR? Not sure what else needed to be done. We need to implement follow up features and this PR has been blocking me for the last 15 days.

cc @bogdandrutu

@alolita
Copy link
Member

alolita commented May 15, 2021

@rakyll Please see @bogdandrutu 's comment above. He did review the PR to merge but wanted some clarification from you.

@rakyll
Copy link
Contributor Author

rakyll commented May 15, 2021

Sorry, just saw that. Addressed.

@alolita
Copy link
Member

alolita commented May 17, 2021

Thanks @rakyll

@bogdandrutu please review and merge.

@bogdandrutu
Copy link
Member

Failing unittests https://github.com/open-telemetry/opentelemetry-collector/pull/3046/checks?check_run_id=2603529811

@rakyll
Copy link
Contributor Author

rakyll commented May 17, 2021

@bogdandrutu rebased.

bogdandrutu
bogdandrutu approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

One last comment. Thank you

exporter/prometheusremotewriteexporter/config.go Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu merged commit 709d8a8 into open-telemetry:main May 17, 2021
@rakyll rakyll deleted the prwexporter-queue-config branch May 18, 2021 01:27
@jpkrohling
Copy link
Member

Sorry, just saw that I was mentioned here. The amount of notifications I get from this repo made me miss it :-/ In the future, if possible, ping me on CNCF Slack and I'll review or assign it to someone.

@alolita alolita mentioned this pull request May 26, 2021
Closed
dashpole pushed a commit to dashpole/opentelemetry-collector that referenced this pull request Jun 14, 2021
@rakyll @dashpole
Allow users to configure the Prometheus remote write queue (open-tele…
cd4d002 
…metry#3046)

* Allow users to configure the Prometheus remote write queue

* Fix lint

* Fix godoc

* Fix docs

* Revert wait group change

* Limit concurrency to the write reqs

* Rename min_shards to concurrency

* Renaming the queue settings

* Renaming the queue settings
@crobert-1 crobert-1 mentioned this pull request Jan 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odeke-em odeke-em odeke-em approved these changes

@dashpole dashpole dashpole approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@gramidt gramidt gramidt approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@anuraaga anuraaga anuraaga approved these changes

Assignees
No one assigned
Labels
ready-to-merge Code review completed; ready to merge by maintainers waiting-for-author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@rakyll @alolita @jpkrohling @bogdandrutu @anuraaga @carlosalberto @Aneurysm9 @gramidt @dashpole @odeke-em