From f4ac89958f466e2d30911e3084450f6b2912639f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dario=20Casta=C3=B1=C3=A9?= Date: Fri, 12 Jul 2024 16:07:47 +0200 Subject: [PATCH 1/3] internal: cache build info to reduce lock contention on initialization --- ddtrace/tracer/tracer_test.go | 14 ++++---- internal/gitmetadata.go | 32 +++++++------------ internal/gitmetadata_test.go | 60 +++++++++++++++++++++++++++++++++++ profiler/upload_test.go | 18 ++++++----- 4 files changed, 88 insertions(+), 36 deletions(-) diff --git a/ddtrace/tracer/tracer_test.go b/ddtrace/tracer/tracer_test.go index 392f152545..19835db105 100644 --- a/ddtrace/tracer/tracer_test.go +++ b/ddtrace/tracer/tracer_test.go @@ -1862,14 +1862,12 @@ func TestEnvironment(t *testing.T) { } func TestGitMetadata(t *testing.T) { - maininternal.ResetGitMetadataTags() - t.Run("git-metadata-from-dd-tags", func(t *testing.T) { t.Setenv(maininternal.EnvDDTags, "git.commit.sha:123456789ABCD git.repository_url:github.com/user/repo go_path:somepath") + maininternal.RefreshGitMetadataTags() tracer, _, _, stop := startTestTracer(t) defer stop() - defer maininternal.ResetGitMetadataTags() assert := assert.New(t) sp := tracer.StartSpan("http.request").(*span) @@ -1882,10 +1880,10 @@ func TestGitMetadata(t *testing.T) { t.Run("git-metadata-from-dd-tags-with-credentials", func(t *testing.T) { t.Setenv(maininternal.EnvDDTags, "git.commit.sha:123456789ABCD git.repository_url:https://user:passwd@github.com/user/repo go_path:somepath") + maininternal.RefreshGitMetadataTags() tracer, _, _, stop := startTestTracer(t) defer stop() - defer maininternal.ResetGitMetadataTags() assert := assert.New(t) sp := tracer.StartSpan("http.request").(*span) @@ -1902,10 +1900,10 @@ func TestGitMetadata(t *testing.T) { // git metadata env has priority over DD_TAGS t.Setenv(maininternal.EnvGitRepositoryURL, "github.com/user/repo_new") t.Setenv(maininternal.EnvGitCommitSha, "123456789ABCDE") + maininternal.RefreshGitMetadataTags() tracer, _, _, stop := startTestTracer(t) defer stop() - defer maininternal.ResetGitMetadataTags() assert := assert.New(t) sp := tracer.StartSpan("http.request").(*span) @@ -1918,10 +1916,10 @@ func TestGitMetadata(t *testing.T) { t.Run("git-metadata-from-env-with-credentials", func(t *testing.T) { t.Setenv(maininternal.EnvGitRepositoryURL, "https://u:t@github.com/user/repo_new") t.Setenv(maininternal.EnvGitCommitSha, "123456789ABCDE") + maininternal.RefreshGitMetadataTags() tracer, _, _, stop := startTestTracer(t) defer stop() - defer maininternal.ResetGitMetadataTags() assert := assert.New(t) sp := tracer.StartSpan("http.request").(*span) @@ -1934,10 +1932,10 @@ func TestGitMetadata(t *testing.T) { t.Run("git-metadata-from-env-and-tags", func(t *testing.T) { t.Setenv(maininternal.EnvDDTags, "git.commit.sha:123456789ABCD") t.Setenv(maininternal.EnvGitRepositoryURL, "github.com/user/repo") + maininternal.RefreshGitMetadataTags() tracer, _, _, stop := startTestTracer(t) defer stop() - defer maininternal.ResetGitMetadataTags() assert := assert.New(t) sp := tracer.StartSpan("http.request").(*span) @@ -1953,10 +1951,10 @@ func TestGitMetadata(t *testing.T) { t.Setenv(maininternal.EnvDDTags, "git.commit.sha:123456789ABCD git.repository_url:github.com/user/repo") t.Setenv(maininternal.EnvGitRepositoryURL, "github.com/user/repo_new") t.Setenv(maininternal.EnvGitCommitSha, "123456789ABCDE") + maininternal.RefreshGitMetadataTags() tracer, _, _, stop := startTestTracer(t) defer stop() - defer maininternal.ResetGitMetadataTags() assert := assert.New(t) sp := tracer.StartSpan("http.request").(*span) diff --git a/internal/gitmetadata.go b/internal/gitmetadata.go index 6dabe6a444..09dc5a971a 100644 --- a/internal/gitmetadata.go +++ b/internal/gitmetadata.go @@ -40,8 +40,7 @@ const ( ) var ( - lock = sync.Mutex{} - + initOnce sync.Once gitMetadataTags map[string]string ) @@ -76,10 +75,10 @@ func getTagsFromDDTags() map[string]string { } } -// getTagsFromBinary extracts git metadata from binary metadata -func getTagsFromBinary() map[string]string { +// getTagsFromBinary extracts git metadata from binary metadata. +func getTagsFromBinary(readBuildInfo func() (*debug.BuildInfo, bool)) map[string]string { res := make(map[string]string) - info, ok := debug.ReadBuildInfo() + info, ok := readBuildInfo() if !ok { log.Debug("ReadBuildInfo failed, skip source code metadata extracting") return res @@ -104,30 +103,23 @@ func getTagsFromBinary() map[string]string { // GetGitMetadataTags returns git metadata tags func GetGitMetadataTags() map[string]string { - lock.Lock() - defer lock.Unlock() - - if gitMetadataTags != nil { - return gitMetadataTags - } + initOnce.Do(initGitMetadataTags) + return gitMetadataTags +} +func initGitMetadataTags() { gitMetadataTags = make(map[string]string) if BoolEnv(EnvGitMetadataEnabledFlag, true) { updateAllTags(gitMetadataTags, getTagsFromEnv()) updateAllTags(gitMetadataTags, getTagsFromDDTags()) - updateAllTags(gitMetadataTags, getTagsFromBinary()) + updateAllTags(gitMetadataTags, getTagsFromBinary(debug.ReadBuildInfo)) } - - return gitMetadataTags } -// ResetGitMetadataTags reset cashed metadata tags -func ResetGitMetadataTags() { - lock.Lock() - defer lock.Unlock() - - gitMetadataTags = nil +// RefreshGitMetadataTags reset cashed metadata tags +func RefreshGitMetadataTags() { + initGitMetadataTags() } // CleanGitMetadataTags cleans up tags from git metadata diff --git a/internal/gitmetadata_test.go b/internal/gitmetadata_test.go index 4577674c63..2e5efb1d12 100644 --- a/internal/gitmetadata_test.go +++ b/internal/gitmetadata_test.go @@ -6,6 +6,7 @@ package internal import ( + "runtime/debug" "testing" "github.com/stretchr/testify/assert" @@ -54,3 +55,62 @@ func TestRemoveCredentials(t *testing.T) { }) } } + +func TestGetTagsFromBinary(t *testing.T) { + testCases := []struct { + name string + in string + expected map[string]string + }{ + { + name: "empty build info", + expected: map[string]string{}, + }, + { + name: "build info with module path", + expected: map[string]string{ + TagGoPath: "github.com/DataDog/dd-trace-go", + }, + }, + { + name: "build info with module path and git repository", + expected: map[string]string{ + TagGoPath: "github.com/DataDog/dd-trace-go", + TagCommitSha: "123456", + }, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + readBuildInfo := func() (*debug.BuildInfo, bool) { + info := &debug.BuildInfo{ + Settings: []debug.BuildSetting{ + { + Key: "vcs", + Value: "git", + }, + }, + } + if tc.expected[TagGoPath] != "" { + info.Path = tc.expected[TagGoPath] + } + if tc.expected[TagCommitSha] != "" { + info.Settings = append(info.Settings, debug.BuildSetting{ + Key: "vcs.revision", + Value: tc.expected[TagCommitSha], + }) + } + return info, true + } + tags := getTagsFromBinary(readBuildInfo) + assert.Subset(t, tags, tc.expected) + }) + } +} + +func BenchmarkGetGitMetadataTags(b *testing.B) { + b.Setenv(EnvGitMetadataEnabledFlag, "true") + for i := 0; i < b.N; i++ { + GetGitMetadataTags() + } +} diff --git a/profiler/upload_test.go b/profiler/upload_test.go index 2e26a5bd70..a5319e6443 100644 --- a/profiler/upload_test.go +++ b/profiler/upload_test.go @@ -120,12 +120,10 @@ func TestEntityContainerIDHeaders(t *testing.T) { } func TestGitMetadata(t *testing.T) { - maininternal.ResetGitMetadataTags() - defer maininternal.ResetGitMetadataTags() - t.Run("git-metadata-from-dd-tags", func(t *testing.T) { - maininternal.ResetGitMetadataTags() t.Setenv(maininternal.EnvDDTags, "git.commit.sha:123456789ABCD git.repository_url:github.com/user/repo go_path:somepath") + maininternal.RefreshGitMetadataTags() + profile := doOneShortProfileUpload(t) assert := assert.New(t) @@ -134,8 +132,9 @@ func TestGitMetadata(t *testing.T) { assert.Contains(profile.tags, "go_path:somepath") }) t.Run("git-metadata-from-dd-tags-with-credentials", func(t *testing.T) { - maininternal.ResetGitMetadataTags() t.Setenv(maininternal.EnvDDTags, "git.commit.sha:123456789ABCD git.repository_url:http://u@github.com/user/repo go_path:somepath") + maininternal.RefreshGitMetadataTags() + profile := doOneShortProfileUpload(t) assert := assert.New(t) @@ -144,12 +143,13 @@ func TestGitMetadata(t *testing.T) { assert.Contains(profile.tags, "go_path:somepath") }) t.Run("git-metadata-from-env", func(t *testing.T) { - maininternal.ResetGitMetadataTags() t.Setenv(maininternal.EnvDDTags, "git.commit.sha:123456789ABCD git.repository_url:github.com/user/repo") // git metadata env has priority under DD_TAGS t.Setenv(maininternal.EnvGitRepositoryURL, "github.com/user/repo_new") t.Setenv(maininternal.EnvGitCommitSha, "123456789ABCDE") + maininternal.RefreshGitMetadataTags() + profile := doOneShortProfileUpload(t) assert := assert.New(t) @@ -157,9 +157,10 @@ func TestGitMetadata(t *testing.T) { assert.Contains(profile.tags, "git.repository_url:github.com/user/repo_new") }) t.Run("git-metadata-from-env-with-credentials", func(t *testing.T) { - maininternal.ResetGitMetadataTags() t.Setenv(maininternal.EnvGitRepositoryURL, "https://u@github.com/user/repo_new") t.Setenv(maininternal.EnvGitCommitSha, "123456789ABCDE") + maininternal.RefreshGitMetadataTags() + profile := doOneShortProfileUpload(t) assert := assert.New(t) @@ -168,11 +169,12 @@ func TestGitMetadata(t *testing.T) { }) t.Run("git-metadata-disabled", func(t *testing.T) { - maininternal.ResetGitMetadataTags() t.Setenv(maininternal.EnvGitMetadataEnabledFlag, "false") t.Setenv(maininternal.EnvDDTags, "git.commit.sha:123456789ABCD git.repository_url:github.com/user/repo") t.Setenv(maininternal.EnvGitRepositoryURL, "github.com/user/repo") t.Setenv(maininternal.EnvGitCommitSha, "123456789ABCD") + maininternal.RefreshGitMetadataTags() + profile := doOneShortProfileUpload(t) assert := assert.New(t) From c3c01f31ad0245bf5ee700ade6969ea9fd2197f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dario=20Casta=C3=B1=C3=A9?= Date: Fri, 12 Jul 2024 16:08:54 +0200 Subject: [PATCH 2/3] internal: fix RefreshGitMetadataTags comment --- internal/gitmetadata.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gitmetadata.go b/internal/gitmetadata.go index 09dc5a971a..6ac38b8673 100644 --- a/internal/gitmetadata.go +++ b/internal/gitmetadata.go @@ -117,7 +117,7 @@ func initGitMetadataTags() { } } -// RefreshGitMetadataTags reset cashed metadata tags +// RefreshGitMetadataTags reset cached metadata tags func RefreshGitMetadataTags() { initGitMetadataTags() } From 3d109862694e26a38297ab5a64a8eb26afc79ad6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dario=20Casta=C3=B1=C3=A9?= Date: Fri, 12 Jul 2024 16:19:49 +0200 Subject: [PATCH 3/3] internal: apply suggestions from code review Co-authored-by: Eliott Bouhana <47679741+eliottness@users.noreply.github.com> --- internal/gitmetadata.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/gitmetadata.go b/internal/gitmetadata.go index 6ac38b8673..ce0ea38e57 100644 --- a/internal/gitmetadata.go +++ b/internal/gitmetadata.go @@ -101,7 +101,7 @@ func getTagsFromBinary(readBuildInfo func() (*debug.BuildInfo, bool)) map[string return res } -// GetGitMetadataTags returns git metadata tags +// GetGitMetadataTags returns git metadata tags. Returned map is read-only func GetGitMetadataTags() map[string]string { initOnce.Do(initGitMetadataTags) return gitMetadataTags @@ -117,7 +117,7 @@ func initGitMetadataTags() { } } -// RefreshGitMetadataTags reset cached metadata tags +// RefreshGitMetadataTags reset cached metadata tags. NOT thread-safe, use for testing only func RefreshGitMetadataTags() { initGitMetadataTags() }