Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to override name of all metrics tables #34225

Closed
Fiery-Fenix opened this issue Jul 23, 2024 · 8 comments
Closed

Add ability to override name of all metrics tables #34225

Fiery-Fenix opened this issue Jul 23, 2024 · 8 comments
Assignees
Labels

Comments

@Fiery-Fenix
Copy link

Component(s)

exporter/clickhouse

Is your feature request related to a problem? Please describe.

Right now table names for metrics are semi-hardcoded, i.e. it's using configurable prefix from metrics_table_name parameter and hardcoded suffix for each metric type, like _gauge/_sum/_summary/etc.
There is no possibility to change this suffixes outside of the code, but this might be useful for such use-cases:

  • Using single table for all metrics types, i.e. otel_metrics, which bring better compression and usability - no need to search for required metric across 5 different tables
  • Ability to have more human-readable naming, for example rename otel_metrics_sum table to otel_metrics_counter which is more comfortable naming for those who is more familiar with Prometheus metrics types

Describe the solution you'd like

Add configuration parameters with metrics tables suffix override.
For example:

  • metrics_table_gauge_suffix: "_gauge"
  • metrics_table_sum_suffix: "_sum"
  • metrics_table_summary_suffix: "_summary"
  • metrics_table_histogram_suffix: "_histogram"
  • metrics_table_exponential_histogram_suffix: "_exponential_histogram"

Another option might be to deprecate current metrics_table_name configuration parameter and introduce 5 new instead:

  • metrics_table_gauge_name: "otel_metrics_gauge"
  • metrics_table_sum_name: "otel_metrics_sum"
  • metrics_table_summary_name: "otel_metrics_summary"
  • metrics_table_histogram_name: "otel_metrics_histogram"
  • metrics_table_exponential_histogram_name: "otel_metrics_exponential_histogram"

Describe alternatives you've considered

No response

Additional context

No response

@Fiery-Fenix Fiery-Fenix added enhancement New feature or request needs triage New item requiring triage labels Jul 23, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@SpencerTorres
Copy link
Member

Hey, thanks for writing this. This is a reasonable request overall, few notes:

Using single table for all metrics types, i.e. otel_metrics, which bring better compression and usability - no need to search for required metric across 5 different tables

I believe this would negatively affect querying, depending on the primary key / aggregation, but the schemas can be replaced by the user so that's not too relevant. I'm unsure if these tables would remain compatible long term. If we change the insert SQL for one kind of metric but not the other, then it wouldn't be possible to use the same table. This can be addressed later however.

Add configuration parameters with metrics tables suffix override.
. . .
Another option might be to deprecate current metrics_table_name configuration parameter and introduce 5 new instead:

If this were to be added, it would be nice to see them under one field:

metrics_table_suffix:
  gauge: "_gauge"
  sum: "_counter"
  summary: "_summary"
  histogram: "_histogram"
  exponential_histogram: "_exponential_histogram"

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.

As a temporary workaround, you could always use a materialized view to forward _sum to _counter.

Thanks for the suggestion! Always happy to improve flexibility. Interested in seeing what others think

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Jul 23, 2024
@odubajDT
Copy link
Contributor

Hello! I would like to take this ticket if possible

@Fiery-Fenix
Copy link
Author

I believe this would negatively affect querying, depending on the primary key / aggregation, but the schemas can be replaced by the user so that's not too relevant. I'm unsure if these tables would remain compatible long term. If we change the insert SQL for one kind of metric but not the other, then it wouldn't be possible to use the same table. This can be addressed later however.

As for performance of single metrics table - on my small set of ~6Bil rows I haven't found statistically significant performance degradation comparing to current, multi-table approach. I know that ClickHouse is very well optimized for smaller amount of tables with bigger size. That's why I was trying to test my sinlge-table approach on production-like environment with bigger amount of data. That's how this issue was born ;)
Related to compatibility with insert SQL queries - as long as single table will have all fields from other 5 tables - there will be 0 issues with this approach. ClickHouse inserts default value (or Null if column is Nullable) in case of absence of this column in insert SQL. I've already tested this approach with custom built OpenTelemetry Collector (just renamed all metrics tables in code) - and it works as expected

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.

Agree, that's looks as a best solution and also aligned with other tables names overrides (like logs and traces)

@Frapschen
Copy link
Contributor

@odubajDT assigned

Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Sep 24, 2024
@SpencerTorres
Copy link
Member

Not stale, waiting on #34251

@github-actions github-actions bot removed the Stale label Sep 27, 2024
mx-psi pushed a commit that referenced this issue Oct 4, 2024
…s for all metric types (#34251)

**Description:** <Describe what has changed.>
- add the ability to override default table names for all metric types

**Link to tracking Issue:** #34225

**Testing:**
- unit tests

**Documentation:** <Describe the documentation added.>

---------

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this issue Oct 4, 2024
…s for all metric types (open-telemetry#34251)

**Description:** <Describe what has changed.>
- add the ability to override default table names for all metric types

**Link to tracking Issue:** open-telemetry#34225

**Testing:**
- unit tests

**Documentation:** <Describe the documentation added.>

---------

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Eromosele-SM pushed a commit to sematext/opentelemetry-collector-contrib that referenced this issue Oct 9, 2024
…s for all metric types (open-telemetry#34251)

**Description:** <Describe what has changed.>
- add the ability to override default table names for all metric types

**Link to tracking Issue:** open-telemetry#34225

**Testing:**
- unit tests

**Documentation:** <Describe the documentation added.>

---------

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@SpencerTorres
Copy link
Member

@crobert-1 solved by #34251, we can close this

@mx-psi mx-psi closed this as completed Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants