sci: add git metadata from artifacts support #1646

sashacmc · 2023-01-04T18:15:29Z

What does this PR do?

Add three environment variables:

DD_GIT_REPOSITORY_URL string, default: empty
DD_GIT_COMMIT_SHA string, default: empty
DD_TRACE_GIT_METADATA_ENABLED bool, default: true

Add following steps:

Check if DD_TRACE_GIT_METADATA_ENABLED is True.
Check if git metadata is already provided by environment variables:

Check DD_GIT_REPOSITORY_URL and DD_GIT_COMMIT_SHA, if they exist, use metadata from them.
Check DD_TAGS, if it contains git.repository_url and git.commit.sha, use metadata from them.

Continue with metadata from artifacts.
Extract git metadata from version information from binary by means of the debug/buildinfo package:
- vcs.revision for commit hash
- path for go module path

During trace telemetry sending:
Add metadata tags to the first span of each chunk:

_dd.git.repository_url for repository URL
_dd.git.commit.sha for commit hash
_dd.go_path for go module path

During profile telemetry sending:
Add metadata tags to each profile:

git.repository_url for repository URL
git.commit.sha to commit hash
go_path to go module path

Motivation

Allow customers easily provide git metadata

Describe how to test/QA your changes

Build app from the cloned git repo with tracer/profiler with go version >= 1.18
Check git.repository_url and git.commit.sha tags on traces/profiles DD pages
Same as 1, but with DD_GIT_REPOSITORY_URL and DD_GIT_COMMIT_SHA with different values (values from env variables must present)
Same as 1, but with DD_TAGS="git.repository_url:some_repo git.commit.sha:some_sha" (values from env variables must present)
Same as 1, but with DD_TRACE_GIT_METADATA_ENABLED=false, tags must not be presented

Reviewer's Checklist

If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
Changed code has unit tests for its functionality.
If this interacts with the agent in a new way, a system test has been added.

pr-commenter · 2023-01-04T18:19:30Z

Benchmarks

Comparing candidate commit e4a44a1 in PR branch sashacmc/embed-build-artifacts with baseline commit 03c9bd3 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

sashacmc · 2023-01-06T13:43:55Z

NB: Don't merge until APM Intake team deploy corresponded changes!

nsrip-dd

Thanks! I'll have a closer look at the profiler part soon, but I have a thought for a different way to work around needing to support Go 1.17.

internal/gitmetadata.go

nsrip-dd

Okay, I looked over this more for the profiling part. I like that you've de-duplicated the tag logic. There's one small change we should do for tags (see my comment) and then we should be good to go for the profiling part of this change :)

profiler/options.go

felixge

Some quick feedback. Didn't do a full review.

internal/gitmetadata.go

internal/gitmetadatabinary.go

internal/gitmetadata.go

nsrip-dd

Thanks! Approved, with one small question about how DD_TAGS is handled an an optional suggestion for making the test code a little shorter.

nsrip-dd · 2023-01-30T15:54:52Z

profiler/upload_test.go

+
+	t.Run("git-metadata-from-dd-tags", func(t *testing.T) {
+		maininternal.ResetGitMetadataTags()
+		os.Setenv(maininternal.EnvDDTags, "git.commit.sha:123456789ABCD git.repository_url:github.com/user/repo go_path:somepath")


These os.Setenv calls followed by defer os.Unsetenv can be replaced by t.Setenv calls. This saves you some lines of code, and also handles corner cases better, like re-setting the env var to its previous value if it had one.

profiler/options.go

felixge

LGTM. A few nits and one warning, but that can be dealt with in a follow-up PR.

profiler/upload_test.go

felixge · 2023-01-31T15:04:52Z

internal/gitmetadata.go

+	res := make(map[string]string)
+	tags := GetGitMetadataTags()
+
+	updateTags(res, TraceTagRepositoryURL, tags[TagRepositoryURL])
+	updateTags(res, TraceTagCommitSha, tags[TagCommitSha])
+	updateTags(res, TraceTagGoPath, tags[TagGoPath])
+
+	return res


NIT: This could be simplified.

Suggested change

res := make(map[string]string)

tags := GetGitMetadataTags()

updateTags(res, TraceTagRepositoryURL, tags[TagRepositoryURL])

updateTags(res, TraceTagCommitSha, tags[TagCommitSha])

updateTags(res, TraceTagGoPath, tags[TagGoPath])

return res

tags := GetGitMetadataTags()

return map[string]string{

TraceTagRepositoryURL: tags[TagRepositoryURL],

TraceTagCommitSha: tags[TagCommitSha],

TraceTagGoPath: tags[TagGoPath],

}

Thanks, but it will not work, updateTags has some checks (e.g. it will not add empty strings)

felixge · 2023-01-31T15:16:33Z

internal/gitmetadata.go

+func updateTags(tags map[string]string, key string, value string) {
+	if _, ok := tags[key]; !ok && value != "" {
+		tags[key] = value
+	}
+}
+
+func updateAllTags(tags map[string]string, newtags map[string]string) {
+	for k, v := range newtags {
+		updateTags(tags, k, v)
+	}
+}


NIT: This code doesn't really seem to benefit from being split into two functions. The naming of the methods implies that there is tag specific logic happening. But it's really just a simple map merging operation. Maybe a rename and putting it all into a single method would be better?

Suggested change

func updateTags(tags map[string]string, key string, value string) {

if _, ok := tags[key]; !ok && value != "" {

tags[key] = value

}

}

func updateAllTags(tags map[string]string, newtags map[string]string) {

for k, v := range newtags {

updateTags(tags, k, v)

}

}

func mergeNewKeys(dst map[string]string, src map[string]string) {

for k, v := range src {

if _, ok := dst[k]; !ok && v != "" {

dst[k] = v

}

}

}

It's also not clear to me why values from src are ignored if they are ""? Is there a real use case for this?

It's also not clear to me why values from src are ignored if they are ""? Is there a real use case for this?

It is for skip empty tags adding, they can appear e.g. because "" is default value for os.Getenv

felixge · 2023-01-31T15:23:29Z

internal/gitmetadata.go

+		updateAllTags(gitMetadataTags, getTagsFromBinary())
+	}
+
+	return gitMetadataTags


WARN: The returned map is mutable which is dangerous. The caller might decide to add their own keys or even overwrite keys by accident.

felixge · 2023-01-31T15:55:27Z

internal/gitmetadatabinary.go

+		return res
+	}
+	res[TagCommitSha] = commitSha
+	res[TagGoPath] = goPath


NIT: No need for extra goPath var.

Suggested change

res[TagGoPath] = goPath

res[TagGoPath] = info.Path

[SC-1158] Add git metadata from artifacts support

sashacmc added this to the Triage milestone Jan 5, 2023

sashacmc changed the title ~~Add git metadata from artifacts support~~ sci: add git metadata from artifacts support Jan 6, 2023

sashacmc marked this pull request as ready for review January 6, 2023 13:41

sashacmc requested a review from a team as a code owner January 6, 2023 13:41

sashacmc requested a review from a team January 6, 2023 13:41

nsrip-dd requested changes Jan 6, 2023

View reviewed changes

internal/gitmetadata.go Outdated Show resolved Hide resolved

sashacmc requested a review from a team as a code owner January 10, 2023 13:59

nsrip-dd requested changes Jan 10, 2023

View reviewed changes

profiler/options.go Outdated Show resolved Hide resolved

sashacmc added 9 commits January 11, 2023 00:05

Add git metadata from artifacts support

565d0c5

Fix lint and check go version

24582fc

Improve old go versions support

aef0ccd

Fix profiler options parsing

e952c08

Remove debug tags

6ffd76e

Use build constraints

83e709b

Move tags setting to spancontext

c082355

Add trecer tests

dcf85eb

Lint + tests fix

26fe827

sashacmc force-pushed the sashacmc/embed-build-artifacts branch from 11adc10 to 26fe827 Compare January 10, 2023 23:05

felixge requested changes Jan 11, 2023

View reviewed changes

internal/gitmetadata.go Outdated Show resolved Hide resolved

internal/gitmetadata.go Outdated Show resolved Hide resolved

sashacmc added 3 commits January 11, 2023 13:18

Fix kay only tag processing

e0aa608

Add space separated and key only tags test

ee29427

Typo fix

ba17616

sashacmc modified the milestone: Triage Jan 11, 2023

sashacmc added 4 commits January 11, 2023 15:47

Profiler test and fix

96611b3

Fix concurrently

5fdd20c

Provide go path along with the repository_url

881d1b6

Lint fix

eaf03ca

sashacmc modified the milestone: Triage Jan 26, 2023

sashacmc requested review from nsrip-dd and felixge January 26, 2023 12:54

sashacmc added 5 commits January 26, 2023 14:27

Some refactoring

6a735d1

Fix tags priority

da6d6ad

Some rafactoring

fa747d2

Don't override go_path from DD_TAGS

8806a94

Cleanup go_path in DD_TAGS

5aaf480

sashacmc modified the milestones: Triage, v1.47.0 Jan 26, 2023

dianashevchenko reviewed Jan 30, 2023

View reviewed changes

internal/gitmetadata.go Outdated Show resolved Hide resolved

internal/gitmetadatabinary.go Outdated Show resolved Hide resolved

internal/gitmetadata.go Show resolved Hide resolved

Update formatting

7f69e59

sashacmc requested a review from dianashevchenko January 30, 2023 11:27

nsrip-dd previously approved these changes Jan 30, 2023

View reviewed changes

Replace "os.Setenv/defer os.Unsetenv" by t.Setenv

7045b91

sashacmc dismissed nsrip-dd’s stale review via 7045b91 January 30, 2023 17:55

nsrip-dd approved these changes Jan 30, 2023

View reviewed changes

rpelliard approved these changes Jan 31, 2023

View reviewed changes

dianashevchenko modified the milestones: v1.47.0, v1.48.0 Jan 31, 2023

dianashevchenko approved these changes Jan 31, 2023

View reviewed changes

Merge branch 'main' into sashacmc/embed-build-artifacts

5aa3649

felixge approved these changes Feb 1, 2023

View reviewed changes

Merge branch 'main' into sashacmc/embed-build-artifacts

e4a44a1

sashacmc modified the milestone: v1.48.0 Feb 1, 2023

dianashevchenko merged commit 7f6f95d into main Feb 1, 2023

dianashevchenko deleted the sashacmc/embed-build-artifacts branch February 1, 2023 14:36

dianashevchenko pushed a commit that referenced this pull request Feb 9, 2023

sci: add git metadata from artifacts support (#1646)

b693b50

[SC-1158] Add git metadata from artifacts support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sci: add git metadata from artifacts support #1646

sci: add git metadata from artifacts support #1646

sashacmc commented Jan 4, 2023 •

edited

Loading

pr-commenter bot commented Jan 4, 2023 •

edited

Loading

sashacmc commented Jan 6, 2023

nsrip-dd left a comment

nsrip-dd left a comment

felixge left a comment

nsrip-dd left a comment

nsrip-dd Jan 30, 2023

sashacmc Jan 30, 2023

felixge left a comment

felixge Jan 31, 2023

sashacmc Feb 1, 2023

felixge Jan 31, 2023

sashacmc Feb 1, 2023

felixge Jan 31, 2023

felixge Jan 31, 2023

sci: add git metadata from artifacts support #1646

sci: add git metadata from artifacts support #1646

Conversation

sashacmc commented Jan 4, 2023 • edited Loading

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

pr-commenter bot commented Jan 4, 2023 • edited Loading

Benchmarks

sashacmc commented Jan 6, 2023

nsrip-dd left a comment

Choose a reason for hiding this comment

nsrip-dd left a comment

Choose a reason for hiding this comment

felixge left a comment

Choose a reason for hiding this comment

nsrip-dd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixge 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashacmc commented Jan 4, 2023 •

edited

Loading

pr-commenter bot commented Jan 4, 2023 •

edited

Loading