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 connection pooling #31127

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Conversation

kevinnoel-be
Copy link
Contributor

@kevinnoel-be kevinnoel-be commented Feb 8, 2024

Description:
Reuse of connections created per database (configured or discovered) vs current behavior to create & close connection per database on each scrape.

Link to tracking Issue:
#30831

Testing:
Updated unit & integration tests. Also, ran locally multiple scenario:

  • no feature gate specified (default): current behavior maintained, connections created/closed on each database per scrape
  • feature gate connection pool enabled, no connection pool config specified (default): reduction of the number of connections created/closed
  • feature gate connection pool enabled, connection pool config tweaked: connections created on first scrape & closed when configured lifetime reached or collector shutdown

Documentation:

  • change log
  • readme for the feature gate & related optional configurations linked to this feature

Note
Checking internally for getting the CLA signed

@kevinnoel-be kevinnoel-be requested a review from a team February 8, 2024 12:48
Copy link

linux-foundation-easycla bot commented Feb 8, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: kevinnoel-be / name: Kevin N. (380908b)

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit

receiver/postgresqlreceiver/client.go Outdated Show resolved Hide resolved
@kevinnoel-be kevinnoel-be force-pushed the config-db-pool branch 2 times, most recently from 65a7ac1 to 4c448c5 Compare February 12, 2024 09:02
@djaglowski
Copy link
Member

@kevinnoel-be, please sign the CLA

@kevinnoel-be
Copy link
Contributor Author

@kevinnoel-be, please sign the CLA

As mentioned in the description, working on it internally to get it signed, please bear with us 😅

Copy link
Contributor

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

@kevinnoel-be
Copy link
Contributor Author

@djaglowski Thanks for you patience, the CLA is now signed!

@djaglowski
Copy link
Member

Thanks @kevinnoel-be. Code looks generally good to me but I think some recent changes upstream have broken compilation: client_factory.go:42:18: cfg.NetAddr undefined

@kevinnoel-be
Copy link
Contributor Author

@djaglowski My bad, didn't check much on my side. Ran tests + integration tests locally before (properly) pushing again this time 😅

@djaglowski
Copy link
Member

My bad, didn't check much on my side.

No worries. There's a decent gap between local tooling and CI, and lots of unrelated activity causing minor conflicts, so it can be very difficult.

Looks like one more check: Generated code is out of date, please run "make generate" and commit the changes in this PR.

@kevinnoel-be
Copy link
Contributor Author

kevinnoel-be commented Feb 29, 2024

Looks like one more check: Generated code is out of date, please run "make generate" and commit the changes in this PR.

Uh, I guess there's an order to things. I did run gci which changed those imports, maybe needs to be fixed in some template to avoid this? I'll take a look

@djaglowski
Copy link
Member

Hmm, I think this may be related to recent changes in out generated test code. I ran make generate locally on this branch and get the same incorrect format. cc: @atoulme

@@ -9,10 +9,12 @@ import (
 	"github.com/stretchr/testify/require"
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/component/componenttest"
-	"go.opentelemetry.io/collector/confmap/confmaptest"
+
 	"go.opentelemetry.io/collector/consumer/consumertest"
 	"go.opentelemetry.io/collector/receiver"
 	"go.opentelemetry.io/collector/receiver/receivertest"
+
+	"go.opentelemetry.io/collector/confmap/confmaptest"
 )
 
 // assertNoErrorHost implements a component.Host that asserts that there were no errors.
Generated code is out of date, please run "make generate" and commit the changes in this PR.

link to failure here

@atoulme
Copy link
Contributor

atoulme commented Feb 29, 2024

You installed mdatagen from core - try make generate in contrib. I'll harmonize the differences between the repos asap.

That or we still have issues with fmt after generation, I thought we had that figured out.

@djaglowski
Copy link
Member

djaglowski commented Mar 1, 2024

Thanks @atoulme. Unfortunately I ran make generate in contrib (and did again just now with the same result).

➜  opentelemetry-collector-contrib (config-db-pool) make generate
cd cmd/mdatagen && go install .
/Applications/Xcode.app/Contents/Developer/usr/bin/make for-all CMD="go generate ./..."
running go generate ./... in root
running go generate ./... in ./cmd/configschema
...
running go generate ./... in ./receiver/postgresqlreceiver
...
running go generate ./... in ./testbed
running go generate ./... in ./testbed/mockdatasenders/mockdatadogagentexporter
➜  opentelemetry-collector-contrib (config-db-pool) git diff                                                                                                                             ✭
diff --git a/receiver/postgresqlreceiver/generated_component_test.go b/receiver/postgresqlreceiver/generated_component_test.go
index 42ffa42608..9b09d44ee2 100644
--- a/receiver/postgresqlreceiver/generated_component_test.go
+++ b/receiver/postgresqlreceiver/generated_component_test.go
@@ -9,10 +9,12 @@ import (
        "github.com/stretchr/testify/require"
        "go.opentelemetry.io/collector/component"
        "go.opentelemetry.io/collector/component/componenttest"
-       "go.opentelemetry.io/collector/confmap/confmaptest"
+
        "go.opentelemetry.io/collector/consumer/consumertest"
        "go.opentelemetry.io/collector/receiver"
        "go.opentelemetry.io/collector/receiver/receivertest"
+
+       "go.opentelemetry.io/collector/confmap/confmaptest"
 )

@atoulme
Copy link
Contributor

atoulme commented Mar 1, 2024

You might want to rebase off the latest main, at least with #31520 the generated files are impacted.

@djaglowski
Copy link
Member

I rebased locally and unfortunately get the same result

@kevinnoel-be
Copy link
Contributor Author

AFAIS the order we see in CI and locally when running generate seems to correspond with what is in the template. I had the "good" idea to run gci target at some point after the generate one which is why this check is failing (as the template doesn't seem to have the "proper" import order and gci didn't ignore the generated file).

TL;DR I'll rebase on Monday after a make generate to fix this PR. To avoid any similar issue I guess the generated template should follow the imposed/configured imports order.

Behind a feature gate as it's a change in behavior.
Improves the connection management by not recreating those endlessly,
especially useful when log_connections & log_disconnections PG configs
are enabled
@kevinnoel-be
Copy link
Contributor Author

Rebased, ran make generate and didn't had any differences this time. Seems this got fixed in the template (spacing)

@djaglowski djaglowski merged commit a57bec6 into open-telemetry:main Mar 4, 2024
142 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 4, 2024
@kevinnoel-be kevinnoel-be deleted the config-db-pool branch March 4, 2024 14:51
DougManton pushed a commit to DougManton/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
**Description:** <Describe what has changed.>
Reuse of connections created per database (configured or discovered) vs
current behavior to create & close connection per database on each
scrape.

**Link to tracking Issue:** 

open-telemetry#30831

**Testing:** 
Updated unit & integration tests. Also, ran locally multiple scenario:
- no feature gate specified (default): current behavior maintained,
connections created/closed on each database per scrape
- feature gate connection pool enabled, no connection pool config
specified (default): reduction of the number of connections
created/closed
- feature gate connection pool enabled, connection pool config tweaked:
connections created on first scrape & closed when configured lifetime
reached or collector shutdown

**Documentation:**
- change log
- readme for the feature gate & related optional configurations linked
to this feature

**Note**
Checking internally for getting the CLA signed
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
**Description:** <Describe what has changed.>
Reuse of connections created per database (configured or discovered) vs
current behavior to create & close connection per database on each
scrape.

**Link to tracking Issue:** 

open-telemetry#30831

**Testing:** 
Updated unit & integration tests. Also, ran locally multiple scenario:
- no feature gate specified (default): current behavior maintained,
connections created/closed on each database per scrape
- feature gate connection pool enabled, no connection pool config
specified (default): reduction of the number of connections
created/closed
- feature gate connection pool enabled, connection pool config tweaked:
connections created on first scrape & closed when configured lifetime
reached or collector shutdown

**Documentation:**
- change log
- readme for the feature gate & related optional configurations linked
to this feature

**Note**
Checking internally for getting the CLA signed
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