Skip to content

[exporter/clickhouse] Fix database creation #38829

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

Conversation

lim123123123
Copy link
Contributor

@lim123123123 lim123123123 commented Mar 20, 2025

Description

In the current version of the exporter, the createDatabase function uses cfg.buildDSN, which appends the database from the config into the dsn that is passed into the clickhouse driver.
As a result, the exporter tries to connect to the database before it is actually created, causing a ClickHouse exception in the start function

This pr couldn't be merged until the integration test fix

Testing

I've added an integration test that checks whether the database was successfully created. It fails now but works with the fix.

@atoulme
Copy link
Contributor

atoulme commented Mar 20, 2025

I only see changes to tests and the example. What is the fix?

@lim123123123
Copy link
Contributor Author

lim123123123 commented Mar 20, 2025

I only see changes to tests and the example. What is the fix?

The fix is in exporter/clickhouseexporter/exporter_logs.go
It's actually pretty small, you just shouldn't pass the database into dsn inside createDatabase function
Sorry, if it's not clear from the pr description

@atoulme
Copy link
Contributor

atoulme commented Mar 20, 2025

OK, that makes sense. Please add a changelog with make chlog-new and mark the PR as ready for review.

Copy link
Contributor

github-actions bot commented Apr 4, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 4, 2025
@lim123123123
Copy link
Contributor Author

I've added the changelog entry. I also disabled the integration tests (as they were before this PR) because they are flaky and the integration tests fix hasn't been merged yet.

Ideally, it would be best to merge the fix first to avoid conflicts (if we merge this PR first, I’ll have to resolve conflicts and go through approvals again). However, the review is taking longer than expected, so let's go ahead and try to fix this bug now

@lim123123123 lim123123123 marked this pull request as ready for review April 14, 2025 09:52
@github-actions github-actions bot removed the Stale label Apr 15, 2025
Copy link
Contributor

github-actions bot commented May 1, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 1, 2025
@lim123123123
Copy link
Contributor Author

Could someone please review the pr? @dmitryax, @Frapschen, @hanjm, @SpencerTorres
@atoulme, what can I do to remove the 'stale' label? The pr is ready for review, but it seems like no one wants to review it.

Copy link
Member

@SpencerTorres SpencerTorres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the problem, but the fix is a bit confusing. Perhaps we should just change buildDSN to not apply the database at all. We always specify it while inserting, so we never really need to switch to the database at the connection level.

This works though, and I want the users to be unblocked. We can merge it as is

@lim123123123
Copy link
Contributor Author

lim123123123 commented May 5, 2025

I see the problem, but the fix is a bit confusing. Perhaps we should just change buildDSN to not apply the database at all. We always specify it while inserting, so we never really need to switch to the database at the connection level.

This works though, and I want the users to be unblocked. We can merge it as is

I understand your idea—it might actually make sense to simply set the database in each query. However, I don't see the database being set in inserts. Inserts only use the table name, so it's not currently possible to just remove the database from buildDSN now

@atoulme atoulme added ready to merge Code review completed; ready to merge by maintainers and removed waiting-for-code-owners labels May 8, 2025
@atoulme atoulme merged commit 43f2959 into open-telemetry:main May 9, 2025
186 of 189 checks passed
@github-actions github-actions bot added this to the next release milestone May 9, 2025
dragonlord93 pushed a commit to dragonlord93/opentelemetry-collector-contrib that referenced this pull request May 23, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
In the current version of the exporter, the createDatabase function uses
cfg.buildDSN, which appends the database from the config into the dsn
that is passed into the clickhouse driver.
As a result, the exporter tries to connect to the database before it is
actually created, causing a ClickHouse exception in the start function

This pr couldn't be merged until [the integration test
fix](open-telemetry#38798)

<!--Describe what testing was performed and which tests were added.-->
#### Testing
I've added an integration test that checks whether the database was
successfully created. It fails now but works with the fix.

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Antoine Toulme <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/clickhouse ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants