[exporter/clickhouse]: Add the ability to override default table names for all metric types

[odubajDT]

Description:

• add the ability to override default table names for all metric types

Link to tracking Issue: #34225

Testing:

• unit tests

Documentation:

[Fiery-Fenix]

Hi @odubajDT, thanks for quick PR regarding my issue.
As we discussed with @SpencerTorres in original issue #34225 - it's better to override full table names than just suffixes:

Between the two solutions you suggested, it would be best to use the full table name and not just the suffix. It has the most flexibility.

[SpencerTorres]

Thanks for submitting this PR!

I agree with what @Fiery-Fenix suggested. We should have it change the full table name, not just the suffix. It's better to have %s correlate with a single symbol in the SQL instead of doing %s%s. In this case the symbol we want to replace is the full table name.

The config + metrics functions will need to be updated to select the proper table name depending on the metric type. We should also consider which default values to provide, as well as how to handle the existing metrics table config field.

I would say the existing metrics table config field takes precedence, and we should only use the new values if that value is unset (empty string).

Let me know what you think

[odubajDT]

Thanks for submitting this PR!

I agree with what @Fiery-Fenix suggested. We should have it change the full table name, not just the suffix. It's better to have %s correlate with a single symbol in the SQL instead of doing %s%s. In this case the symbol we want to replace is the full table name.

The config + metrics functions will need to be updated to select the proper table name depending on the metric type. We should also consider which default values to provide, as well as how to handle the existing metrics table config field.

I would say the existing metrics table config field takes precedence, and we should only use the new values if that value is unset (empty string).

Let me know what you think

Thanks for the response! To wrap it up that I understand it correctly -> the require change would be to keep the original metrics_table_name parameter from the config and introduce new configuration parameters to set the full names (not only suffixes) of the parameters for each metric type? For example something what was already mentioned in the ticket

metrics_tables:
  gauge: "table_gauge"
  sum: "table_counter"
  summary: "table_summary"
  histogram: "table_histogram"
  exponential_histogram: "table_exponential_histogram"

Totally agree with this ^^ approach.

What about a scenario where only some of the metric_tables parameters are set together with metrics_table_name parameter? Let's imagine an input like this

metrics_table_name: "general_table_name"
metrics_tables:
  gauge: "table_gauge"
  sum: "table_counter"
  summary: "table_summary"

Would this be a valid input? If yes, what would be the expected behavior?

In my opinion I would rather restrict using metrics_table_name and metrics_tables together, since it may bring a lot of confusion for the users and additionally a lot of complexity in the code. Also by restricting it, we will avoid a potential behavioral breaking change, since only add new functionality will be added as the metrics_tables parameter, but no changes will be visible for the users already using existing configs with metrics_table_name parameter.

And additional question at the end, the default values (when none of the parameters are set) will stay the same as they are now?

Thanks for the responses! :)

[Fiery-Fenix]

From my perspective - I would say that we should use this logic:

• Keep metrics_table_name with current logic, but mark it as deprecated
• If both metrics_table_name and metrics_tables are specified - log a warning about incompatibility and totally ignore metrics_table_name - use metrics_tables
• Default values should be kept the same as we have right now

Examples:

Case 1

metrics_table_name: "general_table_name"

Logic is the same as now

Case 2

metrics_table_name: "general_table_name"
metrics_tables:
  gauge: "table_gauge"
  sum: "table_counter"
  summary: "table_summary"

Log a warning, ignore metrics_table_name, for absent tables in metrics_tables - use defaults.
For example above it should be:

• for histogram - otel_metrics_histogram
• for exponential_histogram - otel_metrics_exponential_histogram
• for other tables - as provided in config

Case 3

metrics_tables:
  gauge: "table_gauge"
  sum: "table_counter"
  summary: "table_summary"

The same as in Case 2, but without warnings and ignoring old parameter

Using this approach we will avoid breaking changed (except for deprecation of old parameter), existing users will receive totally the same behavior, new functionality will work correctly even if user will mix deprecated and new approach
Happy to hear other possible options

[odubajDT]

From my perspective - I would say that we should use this logic:

* Keep `metrics_table_name` with current logic, but mark it as deprecated

* If both `metrics_table_name` and `metrics_tables` are specified - log a warning about incompatibility and totally ignore `metrics_table_name` - use `metrics_tables`

* Default values should be kept the same as we have right now

Examples:

Case 1

metrics_table_name: "general_table_name"

Logic is the same as now

Case 2

metrics_table_name: "general_table_name"
metrics_tables:
  gauge: "table_gauge"
  sum: "table_counter"
  summary: "table_summary"

Log a warning, ignore metrics_table_name, for absent tables in metrics_tables - use defaults. For example above it should be:

* for `histogram` - `otel_metrics_histogram`

* for `exponential_histogram` - `otel_metrics_exponential_histogram`

* for other tables - as provided in config

Case 3

metrics_tables:
  gauge: "table_gauge"
  sum: "table_counter"
  summary: "table_summary"

The same as in Case 2, but without warnings and ignoring old parameter

Using this approach we will avoid breaking changed (except for deprecation of old parameter), existing users will receive totally the same behavior, new functionality will work correctly even if user will mix deprecated and new approach Happy to hear other possible options

Agree, thanks for clarification! One more suggestion, if we want to deprecate metrics_table_name, we should log a deprecation warning also for case 1, WDYT? Or should we also consider feature gates for this deprecation?

[odubajDT]

Hi @Fiery-Fenix @SpencerTorres can you take a look please? Thanks!

[SpencerTorres]

Overall a great feature for the exporter. I appreciate the changes you made to the DDL where we have full control of the table name from the code. I have several other notes on how we might simplify the overall code flow in the configuration stage.

Thank you for your time and contribution, let me know if you have any other questions!

[odubajDT]

Hey @SpencerTorres thanks for the review! Please have another look after the adaptations

[SpencerTorres]

The internal API for the metrics tables using a configurable struct is a great improvement. Just a few nits on the README to make sure users have a good sample config.

I appreciate your patience and time in updating this PR

[odubajDT]

Hey @SpencerTorres can you please check again? Thank you!

[odubajDT]

Hi @SpencerTorres any updates on this? Thanks!

[SpencerTorres]

Thanks for the updates!

[SpencerTorres]

This should be ready to merge, just need to verify from the other code owners

[odubajDT]

The current test failures are not related to the changes in this PR

[odubajDT]

@mx-psi can you have another look please ?

[odubajDT]

cc @mx-psi