Skip to content

Conversation

SpencerTorres
Copy link
Member

@SpencerTorres SpencerTorres commented Jun 7, 2025

Description

This PR intends to reorganize the code of the exporter without making any user-facing changes to the behavior or config. The goal is to make it more maintainable and revise all previous changes into one cohesive component.

Apologies in advance for the many changes, but I wanted this to be runnable so we can merge it into main. Let me know if we need to split anything out, or make any changes.

Notable changes:

  • All clickhouse-go database calls now use the library's native Go interface instead of the database/sql interface. This is more efficient and easier to configure + test. There was a lot of overhead in converting from database/sql to the driver's native structures, so this has a performance benefit as well as more idiomatic code.
  • Files have been reorganized so that code is in a more logical and easy to find place. (For example, there were some shared functions inside the exporter_logs.go file, even though it was unrelated to logs)
  • SQL is no longer written inside the .go file, and is now using go:embed to embed it from a directory of files.
  • Moved generic code to the internal package to simplify the top level package's exports.
  • Optimized insert code for logs/traces to be more CPU efficient (there were some redundant calls for converting attributes to map, wasting CPU).

See testing section below for notes on test changes.

Testing

  • Mocked database tests were replaced with full integration tests for logs, traces, and metrics.
  • Integration test share the same test container, ensuring quicker testing (no more starting several containers for simple query tests)
  • Preserved goleak checks for integration tests (and unit tests)
  • Integration tests were split into multiple files (these are in *_integration_test.go files)
  • Unit tests are separate from the integration tests (these are in the usual _test.go files)
  • Added tests for new functions/cases
  • Removed irrelevant/unhelpful tests

@SpencerTorres SpencerTorres changed the title [exporter/clickhouseexporter] full refactor for ClickHouse component [chore] [exporter/clickhouseexporter] full refactor for ClickHouse component Jun 7, 2025
@hanjm
Copy link
Member

hanjm commented Jun 8, 2025

Great.

@github-actions github-actions bot requested a review from Frapschen June 10, 2025 19:01
@dmitryax dmitryax merged commit f92a576 into open-telemetry:main Jun 16, 2025
177 checks passed
@github-actions github-actions bot added this to the next release milestone Jun 16, 2025
@SpencerTorres SpencerTorres deleted the clickhouse_full_reorganize branch June 16, 2025 19:32
BenElferink added a commit to odigos-io/odigos that referenced this pull request Jun 17, 2025
## Description

The following PR broke the Clickhouse default DDL link:
open-telemetry/opentelemetry-collector-contrib#40536
This PR locks the link at a specific tree to prevent 404 not found.

Also, the following broken links were fixed:
- MariaDB MySqlConnector for ADO.NET
dmitryax pushed a commit that referenced this pull request Jun 27, 2025
…40547)

#### Description

Depends on refactoring from #40536

Adds JSON support to the exporter.

Summary:
- Added feature gate (`clickhouse.json`, alpha stability) for enabling
the JSON type pipeline. The `Map` based pipeline is still functional and
is the default. ([How to use feature
gates](https://github.com/open-telemetry/opentelemetry-collector/blob/main/featuregate/README.md#controlling-gates))
- The JSON type replaces all `Map` columns with the [new JSON
type](https://clickhouse.com/docs/sql-reference/data-types/newjson).
- JSON is sent to the server as a string.
- Due to how the server stores JSON paths, OTel standard paths such as
`a.b.c` are returned as objects `a { b { c } }` by the server. This can
be changed via server setting in the future to return all data as
flattened paths `a.b.c`.

Let me know if you have any questions, suggestions, etc.

Thanks!

#### Testing

- Added unit tests for related helper functions
- Added integration tests for JSON logs/traces

#### Documentation

- Updated README, changelog
Dylan-M pushed a commit to Dylan-M/opentelemetry-collector-contrib that referenced this pull request Aug 5, 2025
…mponent (open-telemetry#40536)

#### Description

This PR intends to reorganize the code of the exporter without making
any user-facing changes to the behavior or config. The goal is to make
it more maintainable and revise all previous changes into one cohesive
component.

Apologies in advance for the many changes, but I wanted this to be
runnable so we can merge it into main. **Let me know if we need to split
anything out, or make any changes.**

Notable changes:
- All `clickhouse-go` database calls now use the library's native Go
interface instead of the `database/sql` interface. This is more
efficient and easier to configure + test. There was a lot of overhead in
converting from `database/sql` to the driver's native structures, so
this has a performance benefit as well as more idiomatic code.
- Files have been reorganized so that code is in a more logical and easy
to find place. (For example, there were some shared functions inside the
`exporter_logs.go` file, even though it was unrelated to logs)
- SQL is no longer written inside the `.go` file, and is now using
`go:embed` to embed it from a directory of files.
- Moved generic code to the `internal` package to simplify the top level
package's exports.
- Optimized insert code for logs/traces to be more CPU efficient (there
were some redundant calls for converting attributes to `map`, wasting
CPU).

See testing section below for notes on test changes.

#### Testing

- Mocked database tests were replaced with full integration tests for
logs, traces, and metrics.
- Integration test share the same test container, ensuring quicker
testing (no more starting several containers for simple query tests)
- Preserved `goleak` checks for integration tests (and unit tests)
- Integration tests were split into multiple files (these are in
`*_integration_test.go` files)
- Unit tests are separate from the integration tests (these are in the
usual `_test.go` files)
- Added tests for new functions/cases
- Removed irrelevant/unhelpful tests
Dylan-M pushed a commit to Dylan-M/opentelemetry-collector-contrib that referenced this pull request Aug 5, 2025
…pen-telemetry#40547)

#### Description

Depends on refactoring from open-telemetry#40536

Adds JSON support to the exporter.

Summary:
- Added feature gate (`clickhouse.json`, alpha stability) for enabling
the JSON type pipeline. The `Map` based pipeline is still functional and
is the default. ([How to use feature
gates](https://github.com/open-telemetry/opentelemetry-collector/blob/main/featuregate/README.md#controlling-gates))
- The JSON type replaces all `Map` columns with the [new JSON
type](https://clickhouse.com/docs/sql-reference/data-types/newjson).
- JSON is sent to the server as a string.
- Due to how the server stores JSON paths, OTel standard paths such as
`a.b.c` are returned as objects `a { b { c } }` by the server. This can
be changed via server setting in the future to return all data as
flattened paths `a.b.c`.

Let me know if you have any questions, suggestions, etc.

Thanks!

#### Testing

- Added unit tests for related helper functions
- Added integration tests for JSON logs/traces

#### Documentation

- Updated README, changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants