[exporter/clickhouse] Add create_schema option to config
#32282
Conversation
| @@ -0,0 +1,42 @@ | |||
| -- Default Histogram metrics table DDL | |||
|
|
|||
| CREATE TABLE IF NOT EXISTS otel_metrics_histogram ( | |||
There was a problem hiding this comment.
I think we need a unit test to verify the table DDL gennerate by code is as same as the example ones.
it can prevent the example ones allway up-to-date.
There was a problem hiding this comment.
I considered reading the template strings from a file to avoid this completely, but ultimately this DDL is just an example.
As long as the column names and types are the same, the inserts should run correctly.
If we were to add unit tests then I would prefer to just read the SQL in from a file, or maybe embed it in the code at build time, or extract them into individual .go files. Let me know if any of this sounds like a better solution and I will update the PR. 👍
| if err := createDatabase(ctx, e.cfg); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| internal.SetLogger(e.logger) |
There was a problem hiding this comment.
Any reason to drop this?
There was a problem hiding this comment.
It was moved to the top of the function so that it is called before the DDL statements (conditionally) run.
I also considered extracting all of the DDL to its own function outside of start so this separation would be more explicit instead of relying on an early return from ShouldCreateSchema. Let me know if I should extract it to a runDDL type function. 👍
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@Frapschen can you give this another look and check @SpencerTorres's comments? |
|
@SpencerTorres We can put the work discussed in the comments into the next PR and get this one to merge. |
|
@mx-psi ready to merge |
#### 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]>
[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]>
Description:
Adds
create_schemaboolean to config. This flag will disable the automatic creation of the database and tables so that this can be manually managed by the user.Link to tracking Issue:
#29443 (related)
Testing:
Ran integration tests, and added test for verifying undefined + true + false in for the value in the config.
Documentation:
Added config option to README along with new "schema management" section that contains some recommendations for production deployments as well as requirements for maintaining compatibility.
Also added
examples/default_ddl/which contains all of the default DDL that the exporter runs. These are to be used as a starting point for users managing their own schema.