[clickhouse/exporter] Update create schema config option #33694
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Please rebase. I'm not sure why it depends on #33693 |
SpencerTorres
force-pushed
the
update-create-schema-config-option
branch
from
7824a59
to
3936b42
Compare
|
Rebased. Need to fix the next PR but this one should be good now 👍 |
|
Failing test is unrelated, I've opened #33715 to address the failure. |
crobert-1
approved these changes
Jun 21, 2024
dmitryax
reviewed
Jun 22, 2024
dmitryax
reviewed
Jun 22, 2024
Co-authored-by: Dmitrii Anoshin <[email protected]>
Co-authored-by: Dmitrii Anoshin <[email protected]>
SpencerTorres
force-pushed
the
update-create-schema-config-option
branch
from
616c3c3
to
dc1f681
Compare
dmitryax
approved these changes
Jun 24, 2024
greatestusername-splunk
pushed a commit
to greatestusername/opentelemetry-collector-contrib
that referenced
this pull request
Jun 24, 2024
[clickhouse/exporter] Update create schema config option (open-telemetry#33694) **Description:** A follow-up to open-telemetry#32282 that changes `create_schema` from `*bool` to `bool`, while also properly using the default config / factory. **Testing:** - Updated tests --------- Co-authored-by: Dmitrii Anoshin <[email protected]>
TylerHelmuth
pushed a commit
that referenced
this pull request
Jul 1, 2024
…onfig option. (#33614) ### DEPENDS ON #33693, #33694 **Description:** Sets `async_insert` to true by default to enable [asynchronous inserts](https://clickhouse.com/docs/en/optimize/asynchronous-inserts). Because this value is being given a default, I have added a config option under the same name. Keep in mind that if `async_insert` is provided in `endpoint` or `connection_params` it will take precedence and ignore this new config option. This is similar to how the `database` config option behaves. The goal is to provide better insert performance by default, since not all users will know to set it in their DSN URL. This also opens the discussion to ___**whether or not this is a breaking change**___. Depending on the deployment's telemetry throughput, this could be an unexpected change that leads to [`TOO_MANY_PARTS`](https://clickhouse.com/docs/knowledgebase/exception-too-many-parts) errors. I don't expect this to be the case however, but I welcome any discussion about this concern. This PR is being resubmitted with suggestions from @crobert-1 and @dmitryax applied. Here are the extra changes with these suggestions applied: - Extracted unrelated changes into separate PRs - Updated `async_insert` to avoid using a `bool` pointer - Updated tests to be able to support these non-pointer-yet-still-optional test cases **Testing:** Ran integration tests. Also added an abundance of tests to check the behavior of `async_insert` when present in `endpoint`, `connection_params`, and exporter config. **Documentation:** - Updated README for all related changes Unrelated change, also updated README's SQL samples to use `sql` instead of `clickhouse` for the code samples to enable proper syntax highlighting. ClickHouse SQL is compatible with plain SQL.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
EXTRACTED FROM #33614
DEPENDS ON #33693
Description:
A follow-up to #32282 that changes
create_schemafrom*booltobool, while also properly using the default config / factory.Testing: