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.

Sign up for GitHub

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

Jump to bottom

feat: improve clickhouse connection management #18085

Merged
merged 42 commits into from
Mar 10, 2023
Merged

feat: improve clickhouse connection management #18085

merged 42 commits into from
Mar 10, 2023

Conversation

StarpTech
Copy link
Contributor

@StarpTech StarpTech commented Jan 29, 2023 •
edited
Loading

Description:

Improve clickhouse DSN parsing to support all possible configuration options and fixes TLS support.

This will also fix #18079

Link to tracking Issue:

Testing:

Documentation:

TODO

  • Fixing tests

@StarpTech StarpTech requested review from a team and dmitryax as code owners January 29, 2023 00:00
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 29, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@StarpTech
Copy link
Contributor Author

StarpTech commented Jan 30, 2023 •
edited
Loading

Could someone enable CI for this PR?

@runforesight
Copy link

runforesight bot commented Feb 1, 2023 •
edited
Loading

Foresight Summary

    
Major Impacts

build-and-test duration(33 minutes 1 second) has decreased 35 minutes 45 seconds compared to main branch avg(1 hour 8 minutes 46 seconds).
View More Details

⭕  build-and-test-windows workflow has finished in 7 seconds (43 minutes 27 seconds less than main branch avg.) and finished at 9th Mar, 2023.

Job Failed Steps Tests
windows-unittest-matrix -     🔗  N/A See Details
windows-unittest -     🔗  N/A See Details

✅  telemetrygen workflow has finished in 1 minute 14 seconds (1 minute 26 seconds less than main branch avg.) and finished at 9th Mar, 2023.

Job Failed Steps Tests
build-dev -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details
publish-latest -     🔗  N/A See Details

✅  check-links workflow has finished in 1 minute 59 seconds and finished at 9th Mar, 2023.

Job Failed Steps Tests
changed files -     🔗  N/A See Details
check-links -     🔗  N/A See Details

✅  changelog workflow has finished in 3 minutes 3 seconds and finished at 9th Mar, 2023.

Job Failed Steps Tests
changelog -     🔗  N/A See Details

✅  load-tests workflow has finished in 7 minutes 54 seconds (7 minutes 23 seconds less than main branch avg.) and finished at 9th Mar, 2023.

Job Failed Steps Tests
loadtest (TestTraceAttributesProcessor) -     🔗  ✅ 3  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestIdleMode) -     🔗  ✅ 1  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) -     🔗  ✅ 8  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) -     🔗  ✅ 10  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetric10kDPS|TestMetricsFromFile) -     🔗  ✅ 6  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetricResourceProcessor|TestTrace10kSPS) -     🔗  ✅ 12  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestBallastMemory|TestLog10kDPS) -     🔗  ✅ 18  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details

✅  prometheus-compliance-tests workflow has finished in 4 minutes 25 seconds (3 minutes 59 seconds less than main branch avg.) and finished at 9th Mar, 2023.

Job Failed Steps Tests
prometheus-compliance-tests -     🔗  ✅ 21  ❌ 0  ⏭ 0    🔗 See Details

❌  build-and-test workflow has finished in 33 minutes 1 second (35 minutes 45 seconds less than main branch avg.) and finished at 9th Mar, 2023. 1 job failed.

Job Failed Steps Tests
unittest-matrix (1.19, connector) -     🔗  ✅ 98  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, connector) -     🔗  ✅ 98  ❌ 0  ⏭ 0    🔗 See Details
correctness-metrics -     🔗  ✅ 2  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, internal) -     🔗  ✅ 594  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, processor) -     🔗  ✅ 1546  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, internal) -     🔗  ✅ 594  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, extension) -     🔗  ✅ 538  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, processor) -     🔗  ✅ 1546  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, extension) -     🔗  ✅ 538  ❌ 0  ⏭ 0    🔗 See Details
correctness-traces -     🔗  ✅ 17  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, receiver-0) -     🔗  ✅ 2580  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-0) -     🔗  ✅ 2580  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, exporter) -     🔗  ✅ 2481  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, exporter) -     🔗  ✅ 2481  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-1) -     🔗  ✅ 1940  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, receiver-1) -     🔗  ✅ 1940  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, other) -     🔗  ✅ 4728  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, other) -     🔗  ✅ 4728  ❌ 0  ⏭ 0    🔗 See Details
integration-tests -     🔗  ✅ 55  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details
check-collector-module-version -     🔗  N/A See Details
checks -     🔗  N/A See Details
check-codeowners -     🔗  N/A See Details
lint-matrix (receiver-0) -     🔗  N/A See Details
lint-matrix (processor) -     🔗  N/A See Details
lint-matrix (exporter) -     🔗  N/A See Details
lint-matrix (internal) -     🔗  N/A See Details
lint-matrix (other) -     🔗  N/A See Details
build-examples -     🔗  N/A See Details
lint-matrix (receiver-1) -     🔗  N/A See Details
lint-matrix (extension) -     🔗  N/A See Details
lint-matrix (connector) -     🔗  N/A See Details
lint -     🔗  N/A See Details
unittest (1.20) -     🔗  N/A See Details
unittest (1.19) -     🔗  N/A See Details
cross-compile (darwin, amd64) -     🔗  N/A See Details
cross-compile (darwin, arm64) -     🔗  N/A See Details
cross-compile (linux, 386) -     🔗  N/A See Details
cross-compile (linux, amd64) -     🔗  N/A See Details
cross-compile (linux, arm) -     🔗  N/A See Details
cross-compile (linux, arm64) -     🔗  N/A See Details
cross-compile (linux, ppc64le) -     🔗  N/A See Details
cross-compile (windows, 386) -     🔗  N/A See Details
cross-compile (windows, amd64) -     🔗  N/A See Details
build-package (deb) Test deb package     🔗  N/A See Details
build-package (rpm) -     🔗  N/A See Details
windows-msi -     🔗  N/A See Details
publish-dev -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details
publish-check -     🔗  N/A See Details

✅  e2e-tests workflow has finished in 13 minutes (2 minutes 55 seconds less than main branch avg.) and finished at 9th Mar, 2023.

Job Failed Steps Tests
kubernetes-test (v1.26.0) -     🔗  N/A See Details
kubernetes-test (v1.24.7) -     🔗  N/A See Details
kubernetes-test (v1.25.3) -     🔗  N/A See Details
kubernetes-test (v1.23.13) -     🔗  N/A See Details

🔎 See details on Foresight

*You can configure Foresight comments in your organization settings page.

@Frapschen
Copy link
Contributor

I think approver want you to fix those bug separately.

@StarpTech
Copy link
Contributor Author

It’s a lot of changes, sorry. If it’s at all possible, would you please peel off a few of them into separate PRs? One for the clickhouse upgrade, one for the README change, to start.

Hi @atoulme I think this is micromanagement. The README changes are made due to my modifications and the clickhouse upgrade is a patch. If the changes are not welcome, I can revert it. It's important for us that the DSN handling gets merged. The lack of response in the PR reviews prevents me from creating more PRs than necessary 😞

@atoulme
Copy link
Contributor

atoulme commented Mar 6, 2023

Guilty as charged! I'm actually a manager by trade. I would love to help but the changes are a bit on the large side. I am sure others can step in and help. Smaller focused PRs usually get a fast response (not just from me, from others too!). You can bring your PR at a SIG meeting for discussion.

dmitryax
dmitryax reviewed Mar 6, 2023
View reviewed changes
exporter/clickhouseexporter/config.go Outdated Show resolved Hide resolved
cmd/configschema/go.mod Outdated Show resolved Hide resolved
@StarpTech StarpTech requested review from atoulme and removed request for hanjm and pmcollins March 7, 2023 18:04
@github-actions github-actions bot requested a review from pmcollins March 7, 2023 21:40
dmitryax
dmitryax reviewed Mar 7, 2023
View reviewed changes
exporter/clickhouseexporter/README.md Outdated
Comment on lines 265 to 267
- tcp protocol `tcp://addr1:port,tcp://addr2:port` or TLS `tcp://addr1:port,addr2:port?secure=true`
- http protocol `http://addr1:port,http://addr2:port` or https `https://addr1:port,https://addr2:port`
- clickhouse protocol `clickhouse://addr1:port,clickhouse://addr2:port` or TLS `clickhouse://addr1:port,clickhouse://addr2:port?secure=true`
Copy link
Member

Choose a reason for hiding this comment

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

Is the protocol part supposed to be repeated or typo?

Suggested change
- tcp protocol `tcp://addr1:port,tcp://addr2:port` or TLS `tcp://addr1:port,addr2:port?secure=true`
- http protocol `http://addr1:port,http://addr2:port` or https `https://addr1:port,https://addr2:port`
- clickhouse protocol `clickhouse://addr1:port,clickhouse://addr2:port` or TLS `clickhouse://addr1:port,clickhouse://addr2:port?secure=true`
- tcp protocol `tcp://addr1:port,tcp://addr2:port` or TLS `tcp://addr1:port,addr2:port?secure=true`
- http protocol `http://addr1:port,addr2:port` or https `https://addr1:port,addr2:port`
- clickhouse protocol `clickhouse://addr1:port,addr2:port` or TLS `clickhouse://addr1:port,addr2:port?secure=true`

Copy link
Member

Choose a reason for hiding this comment

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

@StarpTech there are no clickhouse protocol, just http/https or native tcp, clickhouse:// is same just same to tcp:// .

https://github.com/ClickHouse/clickhouse-go/blob/08b27884b899f587eb5c509769cd2bdf74a9e2a1/clickhouse_options.go#L303-L315

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But they sell it and it works https://github.com/ClickHouse/clickhouse-go#dsn What's your proposal?

StarpTech and others added 2 commits March 8, 2023 09:49
@StarpTech @dmitryax
Update exporter/clickhouseexporter/README.md
410de0e 
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
@StarpTech
chore: update changelog
a131e5b
@StarpTech StarpTech requested review from hanjm and removed request for atoulme and pmcollins March 8, 2023 14:01
@dmitryax
Copy link
Member

dmitryax commented Mar 8, 2023

@StarpTech please rebase

@StarpTech
Merge remote-tracking branch 'upstream/main' into feat/improve_clickh…
99321d4 
…ouse_conn_mgmnt

# Conflicts:
#	cmd/configschema/go.mod
#	cmd/configschema/go.sum
#	cmd/otelcontribcol/go.mod
#	cmd/otelcontribcol/go.sum
#	go.mod
#	go.sum
@StarpTech
Copy link
Contributor Author

@dmitryax done

@dmitryax dmitryax merged commit 954b49f into open-telemetry:main Mar 10, 2023
@polRk
Copy link

polRk commented Mar 21, 2023

@StarpTech Why did you remove the Schema section?

@StarpTech
Copy link
Contributor Author

@polRk Because we don't want to keep the schema synced in two places.

@polRk
Copy link

polRk commented Mar 21, 2023

@polRk Because we don't want to keep the schema synced in two places.

Where is the second place?

@StarpTech
Copy link
Contributor Author

StarpTech commented Mar 21, 2023 •
edited
Loading

In code. Every schema is in its corresponding go file.
Example: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/clickhouseexporter/exporter_traces.go#L171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@et et et left review comments

@cwegener cwegener cwegener left review comments

@Frapschen Frapschen Frapschen left review comments

@atoulme atoulme atoulme approved these changes

@dmitryax dmitryax dmitryax approved these changes

@mx-psi mx-psi Awaiting requested review from mx-psi

@hanjm hanjm Awaiting requested review from hanjm

Assignees

@codeboten codeboten

Labels
cmd/configschema configschema command cmd/otelcontribcol otelcontribcol command exporter/clickhouse
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clickhouse exporter not working with https
9 participants
@StarpTech @Frapschen @dmitryax @atoulme @polRk @et @cwegener @hanjm @codeboten