feat(clickhouseexporter): Per pipeline JSON support#46553
Conversation
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
|
Well, I'm definitely not a first time contributor 😅 Looks like my CLA has dropped off though. |
| // testLogsCombinedExporter verifies that Map-based and JSON-based logs exporters | ||
| // can operate side-by-side against the same ClickHouse instance. | ||
| func testLogsCombinedExporter(t *testing.T, endpoint string, mapBody bool) { | ||
| overrideJSONStringSetting := func(config *Config) { |
There was a problem hiding this comment.
This is aping the other tests, debated extracting into helpers, but decided against it.
|
If you're proposing control the JSON support using config instead of feature gate, I don't think the feature gate is needed at all. Otherwise, the user will need to really do two actions to ensure the JSON is supported.
Looking into the README.md also shouldn't be enough, adding Just sharing my thoughts, but let's see what codeowners say about it. |
On
I agree, but dropping it is not backwards compatible. If we shifted to the config and ignored the feature flag, this would break prod workloads as soon as you updated. IMO, the config approach I suggest above gives a more graceful way to update. My proposed end state would be that it's controlled only by config. |
Feature gate is in Alpha stage and not enabled by default at all. You could mark it as |
That's a good shout for the issue, will make a follow up. In terms of the feature flag being in alpha, I know of a few places that are currently relying on the feature gate, so felt it would be better to do it in two phases (maybe warn at startup) |
SpencerTorres
left a comment
There was a problem hiding this comment.
Excellent PR! It's indeed hard to share pipelines with/without JSON, so this is a reasonable feature. I agree with the other review comments, we should deprecate the feature flag. Originally this was added under a feature flag because Map was supposed to be deprecated with JSON becoming the primary pipeline, we didn't want to maintain both implementations. Considering we have both implementations now I suppose there's not much harm in adding this config option. Everything looks good, my only nit is I would prefer json over use_json, but that's not a blocker.
Thanks for the PR!
|
I mis-read the code! Ran a test, and it worked as we'd hope. |
|
@SpencerTorres , do you want to approve it with workflow perms so it can run the tests? |
|
/workflow-approve |
|
/workflow-approve |
|
/workflow-approve |
…6553) #### Description Currently, JSON support for ClickHouse is added via a feature flag, so it's turned off or on for _all_ ClickHouse pipelines. This may not be desirable. At this moment, there are a few features where Map might still win out, compared to JSON (at least in the medium term) This shifts the config down to the pipeline level, but also still keeps the feature flag around for now. The behaviour is: * feature flag enabled == always JSON (parity with current) * feature flag disabled && config.use_json == JSON * feature flag disabled && config.!use_json (default) == Map <!--Describe what testing was performed and which tests were added.--> #### Testing Added many tests in the PR! <!--Describe the documentation added.--> #### Documentation
…6553) #### Description Currently, JSON support for ClickHouse is added via a feature flag, so it's turned off or on for _all_ ClickHouse pipelines. This may not be desirable. At this moment, there are a few features where Map might still win out, compared to JSON (at least in the medium term) This shifts the config down to the pipeline level, but also still keeps the feature flag around for now. The behaviour is: * feature flag enabled == always JSON (parity with current) * feature flag disabled && config.use_json == JSON * feature flag disabled && config.!use_json (default) == Map <!--Describe what testing was performed and which tests were added.--> #### Testing Added many tests in the PR! <!--Describe the documentation added.--> #### Documentation
…6553) #### Description Currently, JSON support for ClickHouse is added via a feature flag, so it's turned off or on for _all_ ClickHouse pipelines. This may not be desirable. At this moment, there are a few features where Map might still win out, compared to JSON (at least in the medium term) This shifts the config down to the pipeline level, but also still keeps the feature flag around for now. The behaviour is: * feature flag enabled == always JSON (parity with current) * feature flag disabled && config.use_json == JSON * feature flag disabled && config.!use_json (default) == Map <!--Describe what testing was performed and which tests were added.--> #### Testing Added many tests in the PR! <!--Describe the documentation added.--> #### Documentation
…6553) #### Description Currently, JSON support for ClickHouse is added via a feature flag, so it's turned off or on for _all_ ClickHouse pipelines. This may not be desirable. At this moment, there are a few features where Map might still win out, compared to JSON (at least in the medium term) This shifts the config down to the pipeline level, but also still keeps the feature flag around for now. The behaviour is: * feature flag enabled == always JSON (parity with current) * feature flag disabled && config.use_json == JSON * feature flag disabled && config.!use_json (default) == Map <!--Describe what testing was performed and which tests were added.--> #### Testing Added many tests in the PR! <!--Describe the documentation added.--> #### Documentation
Description
Currently, JSON support for ClickHouse is added via a feature flag, so it's turned off or on for all ClickHouse pipelines.
This may not be desirable.
At this moment, there are a few features where Map might still win out, compared to JSON (at least in the medium term)
This shifts the config down to the pipeline level, but also still keeps the feature flag around for now. The behaviour is:
Testing
Added many tests in the PR!
Documentation