feat: send logs using agent api v2 if available #264

johnstcn · 2024-07-04T09:45:57Z

Closes #260

Removes manually vendored codersdk, we're back on the real deal now
Moves logging setup to package internal/eblog
Modifies logging to use new agent api v2 methods, falling back to PatchLogs
Adds ENVBUILDER_VERBOSE

Tested manually against Coder version 2.8.0, 2.9.0, and 2.13.0.

…nually vendored codersdk

johnstcn · 2024-07-04T12:53:52Z

cmd/envbuilder/main_test.go

-	"github.com/stretchr/testify/require"
-)
-
-func Test_sendLogs(t *testing.T) {


It's no longer easy to test this via integration without standing up an entire Coder instance, so removed this test.

Do we have an integration test for this?

No, not yet. I think this could be a good candidate for a follow-up, but it would mean we need to:

Stand up an in-memory Coder instance

Create an envbuilder template (we currently keep envbuilder templates in coder/coder)

Assert the logs get sent to Coder

It would be fairly heavyweight all in all. What I'd really like is a more lightweight fluentd-like utility that just consumes stdin and forwards to a Coder instance.

go.mod

johnstcn · 2024-07-04T14:14:32Z

internal/eblog/coder.go

+// coderURL and authenticates using token. It then establishes a
+// dRPC connection to the Agent API and begins sending logs.
+// The returned function is used to block until all logs are sent.
+func Coder(ctx context.Context, coderURL *url.URL, token string) (LogFunc, func(), error) {


To remove the possibility of this breaking older users, I can check the Coder version and conditionally return a different implementation that uses the old deprecated endpoint. WDYT?

We can technically break as we are bumping a major version.

I ended up adding support for backward compatibility based on a similar commit in envbox.

johnstcn · 2024-07-05T09:01:39Z

scripts/develop.sh

-    -e GIT_URL=https://github.com/denoland/deno \
-    -e INIT_SCRIPT="bash" \
+    -e ENVBUILDER_GIT_URL=https://github.com/denoland/deno \
+    -e ENVBUILDER_INIT_SCRIPT="bash" \


review: drive-by

johnstcn · 2024-07-05T09:02:56Z

go.mod

 require (
 	cdr.dev/slog v1.6.2-0.20240126064726-20367d4aede6
 	github.com/GoogleContainerTools/kaniko v1.9.2
 	github.com/breml/rootcerts v0.2.10
 	github.com/chainguard-dev/git-urls v1.0.2
+	github.com/coder/coder/v2 v2.10.1-0.20240704130443-c2d44d16a352


this version includes ConnectRPC20 and LogDest

Interesting that it still shows v2.10.1.

Yeah I noticed that too, I think it's related to the last tag on main:

git tag --merged main | sort -V | tail -1 v2.10.0

After that, we started using branches.

Except in go.mod it's v2.10.1?

All I care about is that the commit is present 🤷

mtojek

Mostly nit picks

options.go

scripts/develop.sh

internal/eblog/coder.go

internal/eblog/coder_internal_test.go

dannykopping

LGTM, minor points

go.mod

dannykopping · 2024-07-05T09:34:41Z

go.mod

 require (
 	cdr.dev/slog v1.6.2-0.20240126064726-20367d4aede6
 	github.com/GoogleContainerTools/kaniko v1.9.2
 	github.com/breml/rootcerts v0.2.10
 	github.com/chainguard-dev/git-urls v1.0.2
+	github.com/coder/coder/v2 v2.10.1-0.20240704130443-c2d44d16a352


Interesting that it still shows v2.10.1.

dannykopping · 2024-07-05T09:35:49Z

internal/log/coder.go

+	rpcConnectTimeout  = 10 * time.Second
+	logSendGracePeriod = 10 * time.Second


Nit: I wonder if these should be configurable; at least the timeout feels like it should be.

I'll leave this as a follow up, we may not end up needing an extra knob.

internal/log/coder.go

internal/log/coder_internal_test.go

dannykopping · 2024-07-05T09:47:03Z

cmd/envbuilder/main_test.go

-	"github.com/stretchr/testify/require"
-)
-
-func Test_sendLogs(t *testing.T) {


Do we have an integration test for this?

Closes #260 - Removes manually vendored codersdk, we're back on the real deal now - Moves logging setup to package internal/eblog - Modifies logging to use new agent api v2 methods, falling back to PatchLogs - Adds ENVBUILDER_VERBOSE Tested manually against Coder version 2.8.0, 2.9.0, and 2.13.0. (cherry picked from commit 5be0611)

johnstcn self-assigned this Jul 4, 2024

johnstcn changed the title ~~feat!: use agentsdk.LogSender instead of deprecated method, remove ma…~~ feat!: send logs using agent api v2 Jul 4, 2024

feat!: use agentsdk.LogSender instead of deprecated method, remove ma…

9a0f335

…nually vendored codersdk

johnstcn force-pushed the cj/agentsdk-logs branch from e629778 to 9a0f335 Compare July 4, 2024 14:01

johnstcn commented Jul 4, 2024

View reviewed changes

johnstcn marked this pull request as ready for review July 4, 2024 14:06

johnstcn requested review from mafredri, dannykopping and mtojek July 4, 2024 14:07

johnstcn commented Jul 4, 2024

View reviewed changes

add backward compatibility

92d7da0

johnstcn changed the title ~~feat!: send logs using agent api v2~~ feat: send logs using agent api v2 if available Jul 4, 2024

johnstcn commented Jul 5, 2024

View reviewed changes

mtojek approved these changes Jul 5, 2024

View reviewed changes

options.go Outdated Show resolved Hide resolved

scripts/develop.sh Show resolved Hide resolved

internal/eblog/coder.go Outdated Show resolved Hide resolved

internal/eblog/coder_internal_test.go Outdated Show resolved Hide resolved

johnstcn added 4 commits July 5, 2024 10:22

eblog -> log

46adc8d

defer retryCancel

40e50f1

Test_Coder -> TestCoder

9b404a0

fixup! eblog -> log

847476e

dannykopping approved these changes Jul 5, 2024

View reviewed changes

johnstcn added 2 commits July 5, 2024 11:04

make TestCoder/V2/Err more explicit

ce78ad9

improve meta-logging

fe33fe6

johnstcn merged commit 5be0611 into main Jul 5, 2024
4 checks passed

johnstcn deleted the cj/agentsdk-logs branch July 5, 2024 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: send logs using agent api v2 if available #264

feat: send logs using agent api v2 if available #264

johnstcn commented Jul 4, 2024 •

edited

Loading

johnstcn Jul 4, 2024

dannykopping Jul 5, 2024

johnstcn Jul 5, 2024

johnstcn Jul 4, 2024

matifali Jul 4, 2024

johnstcn Jul 5, 2024

johnstcn Jul 5, 2024

johnstcn Jul 5, 2024

dannykopping Jul 5, 2024

johnstcn Jul 5, 2024 •

edited

Loading

dannykopping Jul 5, 2024

johnstcn Jul 5, 2024

mtojek left a comment

dannykopping left a comment

dannykopping Jul 5, 2024

dannykopping Jul 5, 2024

johnstcn Jul 5, 2024

dannykopping Jul 5, 2024

		rpcConnectTimeout = 10 * time.Second
		logSendGracePeriod = 10 * time.Second

feat: send logs using agent api v2 if available #264

feat: send logs using agent api v2 if available #264

Conversation

johnstcn commented Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn commented Jul 4, 2024 •

edited

Loading

johnstcn Jul 5, 2024 •

edited

Loading