Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

fix/cody-gateway: use keepalive/idle timeout options for Enterprise Portal#63605

Merged
bobheadxi merged 1 commit into
mainfrom
cody-gateway-ep-dial-options
Jul 3, 2024
Merged

fix/cody-gateway: use keepalive/idle timeout options for Enterprise Portal#63605
bobheadxi merged 1 commit into
mainfrom
cody-gateway-ep-dial-options

Conversation

@bobheadxi

Copy link
Copy Markdown
Member

See https://linear.app/sourcegraph/issue/CORE-203 - it seems the default keepalive and idle options are quite aggressive about keeping idle connections around without verifying them. This change tries to configure some options to ensure idle connections aren't retained for a long time.

Test plan

sg start cody-gateway with CODY_GATEWAY_ENTERPRISE_PORTAL_URL: https://enterprise-portal.sgdev.org:443 in override

[   cody-gateway] INFO cody-gateway.sources.worker.handler actor/source.go:154 Completed sync {"TraceId": "b30602af4f6f3269ed438c86ca37edcc", "SpanId": "154e7f8114b27cab", "handle.timeout": "2m0s", "source": "dotcom-product-subscriptions", "sync_duration": "800.20575ms", "seen": 161}
[   cody-gateway] INFO cody-gateway.sources.worker.handler actor/source.go:165 All sources synced {"TraceId": "b30602af4f6f3269ed438c86ca37edcc", "SpanId": "8f226cc9f0159b70", "handle.timeout": "2m0s"}

@bobheadxi bobheadxi requested review from a team July 2, 2024 21:56
@cla-bot cla-bot Bot added the cla-signed label Jul 2, 2024
Comment on lines +47 to +48
opts = defaults.ExternalDialOptions(logger,
append([]grpc.DialOption{creds}, cloudRunDialOptions...)...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

any reason to not do it this way?

Suggested change
opts = defaults.ExternalDialOptions(logger,
append([]grpc.DialOption{creds}, cloudRunDialOptions...)...)
opts = defaults.ExternalDialOptions(logger, append(cloudRunDialOptions, creds)...)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It matches the callsite above where creds is first 😆

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let's keep this as is and test it out - if it helps, I want to move it into the shared package as a "these are options you should use for interacting with MSP services"

@bobheadxi bobheadxi merged commit ad03371 into main Jul 3, 2024
@bobheadxi bobheadxi deleted the cody-gateway-ep-dial-options branch July 3, 2024 15:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants