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

receiver: Added Docs for Receiver Split behaviour #4316

Closed
wants to merge 9 commits into from

Conversation

yashrsharma44
Copy link
Contributor

@yashrsharma44 yashrsharma44 commented Jun 7, 2021

Added some section about receiver's dual behaviour, which implements the Split Proposal

Also added a Changelog for the PR #4231 because it was not added 😬 .
Signed-off-by: Yash Sharma [email protected]

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
@yashrsharma44 yashrsharma44 force-pushed the receiver-docs branch 2 times, most recently from e86a8ad to 1d90278 Compare June 7, 2021 14:49
docs/components/receive.md Outdated Show resolved Hide resolved
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
@yeya24 yeya24 mentioned this pull request Jun 7, 2021
2 tasks
docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
yashrsharma44 and others added 2 commits June 7, 2021 20:47
Co-authored-by: Lucas Servén Marín <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few small comments.

docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
Signed-off-by: Yash Sharma <[email protected]>
@@ -15,6 +15,14 @@ Thanos Receive supports multi-tenancy by using labels. See [Multitenancy documen
For more information please check out [initial design proposal](../proposals/201812_thanos-remote-receive.md).
For further information on tuning Prometheus Remote Write [see remote write tuning document](https://prometheus.io/docs/practices/remote_write/).

As of version [v0.22.0](https://github.com/thanos-io/thanos/blob/main/CHANGELOG.md#added), the Thanos receiver implements the [receiver split proposal](https://github.com/thanos-io/thanos/blob/main/docs/proposals/202012_receive_split.md), which allows data ingestion as well as request routing to be independently enabled/disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would enable users to prepare a topology of receivers, containing trees of receiver with depth N.

Why would users want to do that? The documentation is the place that users will look for guidance on this topic.

One thing that comes to mind is that this lets the users achieve hard tenancy of receivers - no two customers will share receiver ingest instances.

Copy link
Contributor

@bill3tt bill3tt Jun 11, 2021

Choose a reason for hiding this comment

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

The receiver split proposal does a good job of explaining why.

With the changes coming in in v0.22.0, only routing-receivers need to watch for configuration changes, and ingesting-receivers do not need to. So when the configuration is updated, the routing-receivers will almost instantaneously re-configure without a lengthy WAL flush (& downtime) like we have in the current setup (routing & ingesting receivers).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I add a line at the end -

If you are curious about why you should setup your receiver topology, please refer the proposal for more details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the changes coming in in v0.22.0, only routing-receivers need to watch for configuration changes, and ingesting-receivers do not need to. So when the configuration is updated, the routing-receivers will almost instantaneously re-configure without a lengthy WAL flush (& downtime) like we have in the current setup (routing & ingesting receivers).

Also, I think we can add your description in the doc. The above statement sounds descriptive about the use case. cc @squat WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to use full sentences, otherwise sgtm (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to use full sentences, otherwise sgtm (:

I didn't get which sentence we should complete, can you please explain 😅

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks 💪🏽 Super useful! Some minor comments only!

@@ -15,6 +15,14 @@ Thanos Receive supports multi-tenancy by using labels. See [Multitenancy documen
For more information please check out [initial design proposal](../proposals/201812_thanos-remote-receive.md).
For further information on tuning Prometheus Remote Write [see remote write tuning document](https://prometheus.io/docs/practices/remote_write/).

As of version [v0.22.0](https://github.com/thanos-io/thanos/blob/main/CHANGELOG.md#added), the Thanos receiver implements the [receiver split proposal](https://github.com/thanos-io/thanos/blob/main/docs/proposals/202012_receive_split.md), which allows data ingestion as well as request routing to be independently enabled/disabled.

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to use full sentences, otherwise sgtm (:


We have not added another flag for controlling this behaviour. Instead, we have relied on the existing flags and the below convention to enable and disable the ingestion and routing functionalities:
* **Ingestion and Routing** - Make sure that `--receive.local-endpoint` and either `--receive.hashrings` or `--receive.hashrings-file` are provided.
* **Ingestion** - Since routing behaviour is not needed, provide `--receive.local-endpoint` only.
Copy link
Contributor

@bill3tt bill3tt Jun 15, 2021

Choose a reason for hiding this comment

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

Since --receive.local-endpoint is only used to figure out if the current instance is in the specified hashring, specifying --receive.local-endpoint without also --receive.hashring kind of doesn't make sense to me. It has not effect on the operation of the component at least!

In my opinion it is much clearer for the user if we advise them to enable ingestion mode without specifying any extra parameters.

Also - my preference would re-arrange these bullets so that number of extra parameters are increasing as the list goes down:

  • Ingestion - no parameters.
  • Routing - hashring parameters
  • Ingestion & Routing - hashrigh parameters & local endpoint parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do you propose to have a default value for --receive.local-endpoint, as it was earlier - link?

@stale
Copy link

stale bot commented Aug 17, 2021

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Aug 17, 2021
@stale stale bot closed this Aug 24, 2021
@jmichalek132 jmichalek132 mentioned this pull request Nov 8, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants