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/clickhouseexporter]: persistent queue support #28579

Conversation

fredthomsen
Copy link
Contributor

@fredthomsen fredthomsen commented Oct 24, 2023

Addresses #27653.

Description:
Added persistent storage queue support by leveraging default exporthelper.QueueSettings config structure.

NOTE This does end up being a breaking change to the API.

Link to tracking Issue:
#27653

Testing:
Adjust existing unit test.

@fredthomsen fredthomsen requested a review from a team October 24, 2023 16:22
@fredthomsen fredthomsen force-pushed the clickhouseExporterPersistentQueueEnforced branch 4 times, most recently from d46061f to e2f181d Compare October 24, 2023 17:34
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Minor comments, LGTM otherwise

@fredthomsen fredthomsen force-pushed the clickhouseExporterPersistentQueueEnforced branch from e2f181d to 3861989 Compare October 26, 2023 17:43
@dmitryax
Copy link
Member

@hanjm, @Frapschen, PTAL. Would there be any problem if users configure multiple consumers? If so, we should document it.

@fredthomsen fredthomsen force-pushed the clickhouseExporterPersistentQueueEnforced branch from 3861989 to da1294b Compare October 26, 2023 18:24
@fredthomsen fredthomsen force-pushed the clickhouseExporterPersistentQueueEnforced branch from da1294b to f0bd0bf Compare October 27, 2023 14:37
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM but I would like codeowners, @hanjm and @Frapschen, to take a look before merging

@dmitryax dmitryax merged commit 3d94380 into open-telemetry:main Oct 29, 2023
83 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 29, 2023
@cwegener
Copy link
Contributor

@hanjm, @Frapschen, PTAL. Would there be any problem if users configure multiple consumers? If so, we should document it.

FWIW. I have been running the clickhouseexporter with a custom patch pretty much identical to the patch implemented here.

With one difference, I also removed the single consumer limit and ran with the default from the exporterhelper (10 consumers??), resulting in significant increase in overall pipeline throughput capacity.

Thanks @fredthomsen for creating this PR!

jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
…8579)

Addresses open-telemetry#27653.

**Description:**
Added persistent storage queue support by leveraging default
`exporthelper.QueueSettings` config structure.

**NOTE** This does end up being a **breaking** change to the API.
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this pull request Nov 24, 2023
…8579)

Addresses open-telemetry#27653.

**Description:**
Added persistent storage queue support by leveraging default
`exporthelper.QueueSettings` config structure.

**NOTE** This does end up being a **breaking** change to the API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants