Add Datadog profiler #314

daxmc99 · 2022-03-25T00:28:35Z

Add Datadog as a supported profiler
~~- Print an error if GOMAXPROCs fails~~

Adds Datadog profiler
Part of #294

With cgroups v2 becoming more common, this may not longer work for some container runtimes

keegancsmith

Nice. I have a few inline comments. I only comment in one place the feedback applies, but most of my inline comments apply to other files the pattern was applied.

cmd/zoekt-archive-index/main.go

cmd/zoekt-sourcegraph-indexserver/main.go

keegancsmith · 2022-03-25T03:23:47Z

cmd/zoekt-sourcegraph-indexserver/main.go

 	runtime.SetBlockProfileRate(conf.blockProfileRate)

+	err = profiler.Init(ServiceName, zoekt.Version, conf.blockProfileRate)


why did you move profiler.Init to here? Previously it happened in startServer. I believe that was intentional so we didn't capture short lived debug invocations of this binary.

My thought here was to keep this close to the runtime.SetBlockProfile. If all the Datadog profiler is enabled with DD_MAX_PROFILER then it will set this value.

I suppose I don't fully understand why we are setting the BlockProfileRate here. I'm lacking the context of why this would be set when not profiling or what affect this value would have before the change.

As far as capturing short-lived invocations, I'm not sure I follow. In all debug scenarios, I think you will need to setup an external profiler for profiles to be collected and analyzed.

zoekt-sourcegraph-indexserver can be run not as a daemon, but as a short lived command just for debug purposes. Those do not need datadog/cloud profiler and shouldn't have them.

I think you should move it to where we had it before. runtime.SetBlockProfile doesn't start the block profiler, that is why it was done where ti was before.

cmd/zoekt-webserver/main.go

keegancsmith · 2022-03-25T03:25:26Z

internal/profiler/profiler.go

+		if os.Getenv("DD_PROFILE_ALL") != "" || blockProfileRate > 0 {
+			profileTypes = append(profileTypes, ddprofiler.MutexProfile, ddprofiler.BlockProfile)
+		}
+		return ddprofiler.Start(


you are doing an early return here, and then won't start google cloud profiler.

I think we want only one profiler enabled at a time

Why? If that is what an operator wants, then they can only enable one envvar?

keegancsmith · 2022-03-25T03:35:24Z

Test failure looks unrelated. cc @ggilmore

--- FAIL: TestDeltaShards (0.63s)
    --- FAIL: TestDeltaShards/tombstones_affect_document_across_branches (0.12s)
Error:         e2e_test.go:753: step "tombstone foo": finishing builder: repository metadata in shard "/tmp/TestDeltaShardstombstones_affect_document_across_branches1240476489/001/repository_v16.00000.zoekt" contains a different set of branch names than what was requested, which is unsupported in a delta shard build. old: [{Name:main Version:} {Name:release Version:}], new: [{Name:release Version:} {Name:main Version:}]

This reverts commit bb666dd.

keegancsmith · 2022-03-25T13:07:13Z

cmd/zoekt-sourcegraph-indexserver/main.go

 	runtime.SetBlockProfileRate(conf.blockProfileRate)

+	profiler.Init("zoekt-sourcegraph-indexserver", zoekt.Version, conf.blockProfileRate)


can you just make it so that we call runtime.SetBlockProfileRate inside of profiler.Init? That way webserver also gets to benefit from it.

cc @ggilmore do we actually use this anymore? Should we just get rid of it?

Are you talking about the block profiler itself? We don't run it continuously, but we have the machinery to enable it by setting an environment variable (this would we be useful while we're debugging something).

internal/profiler/profiler.go

ggilmore · 2022-03-29T18:51:00Z

Test failure looks unrelated. cc @ggilmore

--- FAIL: TestDeltaShards (0.63s)
    --- FAIL: TestDeltaShards/tombstones_affect_document_across_branches (0.12s)
Error:         e2e_test.go:753: step "tombstone foo": finishing builder: repository metadata in shard "/tmp/TestDeltaShardstombstones_affect_document_across_branches1240476489/001/repository_v16.00000.zoekt" contains a different set of branch names than what was requested, which is unsupported in a delta shard build. old: [{Name:main Version:} {Name:release Version:}], new: [{Name:release Version:} {Name:main Version:}]

This should be fixed if you merge in master.

Co-authored-by: Keegan Carruthers-Smith <[email protected]>

daxmc99 added 2 commits March 24, 2022 17:15

Add Datadog as a supported profiler

9b80f30

Print an error if GOMAXPROCs fails

bb666dd

With cgroups v2 becoming more common, this may not longer work for some container runtimes

cla-bot bot added the cla-signed label Mar 25, 2022

sp

7da02b0

daxmc99 mentioned this pull request Mar 25, 2022

Add Datadog tracer as a supported tracer #315

Merged

keegancsmith requested changes Mar 25, 2022

View reviewed changes

go mod tidy

180619b

daxmc99 added 5 commits March 24, 2022 21:02

Revert "Print an error if GOMAXPROCs fails"

775808d

This reverts commit bb666dd.

review comments

775f45f

review comments pt2

877e8aa

Review comments v3

32ea12a

review comments v4

f8d27c3

daxmc99 requested a review from keegancsmith March 25, 2022 05:53

keegancsmith approved these changes Mar 25, 2022

View reviewed changes

daxmc99 and others added 3 commits March 29, 2022 12:53

Merge branch 'master' into dax/add_dd_profiler

8ff1af6

Update internal/profiler/profiler.go

c92a228

Co-authored-by: Keegan Carruthers-Smith <[email protected]>

Move profiler back to where it was

5fbe4e0

daxmc99 force-pushed the dax/add_dd_profiler branch from a72f514 to 5fbe4e0 Compare April 5, 2022 21:36

daxmc99 merged commit 2a5b562 into master Apr 5, 2022

daxmc99 deleted the dax/add_dd_profiler branch April 5, 2022 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Datadog profiler #314

Add Datadog profiler #314

daxmc99 commented Mar 25, 2022 •

edited

Loading

keegancsmith left a comment

keegancsmith Mar 25, 2022

daxmc99 Mar 25, 2022 •

edited

Loading

keegancsmith Mar 25, 2022

keegancsmith Mar 25, 2022

daxmc99 Mar 25, 2022

keegancsmith Mar 25, 2022

keegancsmith commented Mar 25, 2022

keegancsmith Mar 25, 2022

ggilmore Mar 29, 2022

ggilmore commented Mar 29, 2022

		runtime.SetBlockProfileRate(conf.blockProfileRate)

		err = profiler.Init(ServiceName, zoekt.Version, conf.blockProfileRate)

		runtime.SetBlockProfileRate(conf.blockProfileRate)

		profiler.Init("zoekt-sourcegraph-indexserver", zoekt.Version, conf.blockProfileRate)

Add Datadog profiler #314

Add Datadog profiler #314

Conversation

daxmc99 commented Mar 25, 2022 • edited Loading

keegancsmith left a comment

Choose a reason for hiding this comment

keegancsmith Mar 25, 2022

Choose a reason for hiding this comment

daxmc99 Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

keegancsmith Mar 25, 2022

Choose a reason for hiding this comment

keegancsmith Mar 25, 2022

Choose a reason for hiding this comment

daxmc99 Mar 25, 2022

Choose a reason for hiding this comment

keegancsmith Mar 25, 2022

Choose a reason for hiding this comment

keegancsmith commented Mar 25, 2022

keegancsmith Mar 25, 2022

Choose a reason for hiding this comment

ggilmore Mar 29, 2022

Choose a reason for hiding this comment

ggilmore commented Mar 29, 2022

daxmc99 commented Mar 25, 2022 •

edited

Loading

daxmc99 Mar 25, 2022 •

edited

Loading