[exporter/clickhouse] Default async_insert to true. Added related config option.
#33614
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
|
Note for future reference, this is a continuation of #32340 |
dmitryax
reviewed
Jun 19, 2024
|
Looks like the README got merged incorrectly, I've removed the duplicate 👍 |
dmitryax
reviewed
Jun 19, 2024
This was referenced Jun 21, 2024
|
@dmitryax did some git surgery to hopefully extract all these changes into separate PRs. The PRs do build on one another, so I've ordered them so that they can be merged without breaking main. |
SpencerTorres
force-pushed
the
config-async-inserts
branch
2 times, most recently
from
902da38 to
ed997a9
Compare
dmitryax
added a commit
that referenced
this pull request
Jun 24, 2024
#### EXTRACTED FROM #33614 #### DEPENDS ON #33693 **Description:** A follow-up to #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]>
SpencerTorres
force-pushed
the
config-async-inserts
branch
2 times, most recently
from
8b9ee99 to
8102c6e
Compare
|
@hanjm @Frapschen PTAL |
dmitryax
approved these changes
Jun 26, 2024
|
Thanks for reviewing! I don't have permission to merge, but after this is merged we can do #33615 |
crobert-1
added
the
ready to merge
TylerHelmuth
reviewed
Jul 1, 2024
TylerHelmuth
removed
the
ready to merge
SpencerTorres
force-pushed
the
config-async-inserts
branch
from
8102c6e to
1b04615
Compare
|
Latest suggestions applied + rebased, hopefully we can get this merged asap since it's been open for a while. Thanks! |
TylerHelmuth
approved these changes
Jul 1, 2024
andrzej-stencel
pushed a commit
that referenced
this pull request
Jul 3, 2024
…33615) **Description:** *NOTE: Depends on merging of #33611, #33614, and #33648* Logs configuration is unlikely to change further with the new recommendation of [manual schema config](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/clickhouseexporter#schema-management). The logs component of the exporter has proven stability, as seen in deployments such as [ClickHouse's Loghouse](https://clickhouse.com/blog/building-a-logging-platform-with-clickhouse-and-saving-millions-over-datadog). With these points in mind, I suggest we upgrade the status to `beta` for logs. Traces and metrics can remain `alpha` for now. **Documentation:** - Updated metadata.yaml - Updated README - Small unrelated change: added/refactored a section that covers tool visualization
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.
DEPENDS ON #33693, #33694
Description:
Sets
async_insertto true by default to enable 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_insertis provided inendpointorconnection_paramsit will take precedence and ignore this new config option.This is similar to how the
databaseconfig 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_PARTSerrors. 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:
async_insertto avoid using aboolpointerTesting:
Ran integration tests. Also added an abundance of tests to check the behavior of
async_insertwhen present inendpoint,connection_params, and exporter config.Documentation:
Unrelated change, also updated README's SQL samples to use
sqlinstead ofclickhousefor the code samples to enable proper syntax highlighting. ClickHouse SQL is compatible with plain SQL.