[exporter/clickhouse] Integration test hits a panic #32530
Comments
|
Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
|
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 Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
|
Not sure if we have any ideas to improve reliability for these tests, definitely want to keep this issue open though |
|
I would like to group this issue with #32275 since they both reference the integration tests, we need to revisit these and improve stability. I have it working locally, but the CI seems unreliable. |
As far as grouping together, it's nice to have open issues for different test failures to track frequencies independently. It's totally fine to open a single PR that fixes both issues though for something like this, if that works for you. |
|
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 Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
|
Still need to investigate integration test stability further. For now these are disabled, and when I manually run them they work fine. Let me know if anyone else here has any ideas |
|
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 Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
|
This issue has been closed as inactive because it has been stale for 120 days with no activity. |
…tests (#38798) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description clickhouse exporter tests use their own sql drivers to validate the queries sent during the tests. Each driver has a unique name (which is the same as the test name). This name must be passed to the sql.Open function during database initialization. Unfortunately, instead of explicitly passing the driver name into the buildDB function, it was passed implicitly through a global variable. This approach works in production but leads to data races when tests are executed in parallel because each test writes its own name to driverName. As a result, integration tests expected driverName to be set to the standard "clickhouse", but instead, it retained the value set by the previous test. This caused the validation callbacks to be triggered incorrectly, leading to test failures (panics). There were two possible solutions to fix this issue: 1. Hardcode the "clickhouse" constant in integration tests. 2. Pass driverName explicitly into sql.Open. Although the first option might work, I believe it is not a careful way to write tests and code. A global variable that changes during tests introduces bugs that are difficult to debug and reproduce. Therefore, I prefer the second option. <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes Close #32530 --------- Co-authored-by: Antoine Toulme <[email protected]>
…tests (open-telemetry#38798) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description clickhouse exporter tests use their own sql drivers to validate the queries sent during the tests. Each driver has a unique name (which is the same as the test name). This name must be passed to the sql.Open function during database initialization. Unfortunately, instead of explicitly passing the driver name into the buildDB function, it was passed implicitly through a global variable. This approach works in production but leads to data races when tests are executed in parallel because each test writes its own name to driverName. As a result, integration tests expected driverName to be set to the standard "clickhouse", but instead, it retained the value set by the previous test. This caused the validation callbacks to be triggered incorrectly, leading to test failures (panics). There were two possible solutions to fix this issue: 1. Hardcode the "clickhouse" constant in integration tests. 2. Pass driverName explicitly into sql.Open. Although the first option might work, I believe it is not a careful way to write tests and code. A global variable that changes during tests introduces bugs that are difficult to debug and reproduce. Therefore, I prefer the second option. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes Close open-telemetry#32530 --------- Co-authored-by: Antoine Toulme <[email protected]>
…tests (open-telemetry#38798) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description clickhouse exporter tests use their own sql drivers to validate the queries sent during the tests. Each driver has a unique name (which is the same as the test name). This name must be passed to the sql.Open function during database initialization. Unfortunately, instead of explicitly passing the driver name into the buildDB function, it was passed implicitly through a global variable. This approach works in production but leads to data races when tests are executed in parallel because each test writes its own name to driverName. As a result, integration tests expected driverName to be set to the standard "clickhouse", but instead, it retained the value set by the previous test. This caused the validation callbacks to be triggered incorrectly, leading to test failures (panics). There were two possible solutions to fix this issue: 1. Hardcode the "clickhouse" constant in integration tests. 2. Pass driverName explicitly into sql.Open. Although the first option might work, I believe it is not a careful way to write tests and code. A global variable that changes during tests introduces bugs that are difficult to debug and reproduce. Therefore, I prefer the second option. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes Close open-telemetry#32530 --------- Co-authored-by: Antoine Toulme <[email protected]>
Component(s)
exporter/clickhouse
Describe the issue you're reporting
Failing CI/CD link
For context, some of these tests haven't been run for a while (if ever), so this may not be a recent break. See #32207.
Failure output:
The text was updated successfully, but these errors were encountered: