[A-1511] Add OTel spans for repo-checkout git operations

[isaacsu]

Description

Emit additional git.* OpenTelemetry spans so customers can see what goes on behind the "Preparing working directory" / repo-checkout span and figure out ways to speed up the checkout phase of their jobs.

Spans added

Span	Description
repo-checkout (existing)	Added attempt counter; git.* builds off its child context
git.mirror.update	main repo getOrUpdateMirrorDir
└ git.mirror.lock_wait.clone	mirror clonelock
└ git.mirror.clone	mirror gitClone
└ git.mirror.lock_wait.update	mirror updatelock
└ git.mirror.fetch	mirror gitFetch
└ git.mirror.snapshot	snapshot gitClone
git.dissociate	main repo gitRepack if GitMirrorCheckoutMode is dissociate
git.clone	main repo gitClone
git.clean.pre	main repo gitCleanSubmodules and gitClean
git.lfs.install	git lfs install
git.fetch	main repo fetchSource
git.verify_commit	main repo verifyCommit
git.sparse_checkout	setupSparseCheckout
git.checkout	main repo gitCheckout
git.submodules	main repo updateGitSubmodules
└ (for each submodule)
└ git.mirror.update	submodule getOrUpdateMirrorDir (see git.mirror.update above)
└ git.submodule.update	git submodule update
git.lfs.fetch	main repo gitLFSFetchCheckout
git.clean.post	main repo gitClean and gitCleanSubmodules

Screenshot

Changes

• Add instrumentation to internal/job/checkout*.go paths
• Add traceOp*helper functions to internal/job/tracing.go
• Add/update tests for this change.
• Builds upon
  • [A-1511] Redact repo URL credentials in repo-checkout OTel span #4099
  • [A-1511] Split internal/job/checkout.go into multiple files #4109
  • [A-1511] Extract updateGitSubmodules into its own method #4111
  • [A-1511] Refactor refspec classification in fetchSource #4117

Testing

• Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
• Code is formatted (with go tool gofumpt -extra -w .)

Disclosures / Credits

While this PR was produced with the help of advanced automation tools, I take full credit and responsibility for it.

[zhming0]

More Otel span the better 🙌🏿 , but I wonder if we can make them less invasive in our code, perhaps we can start by thinking if the flag is worth it.

[buildsworth-bk-app]

I didn't find a concrete defect in the latest revision. Making the git.* spans automatic for either tracing backend across the default checkout path is cross-cutting enough to warrant a human sanity-check.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 8013, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[zhming0]

[not blocking] I wonder if this previous attempts attribute is reasonable. Because when we are looking at Otel Spans, they show up in waterfall diagram and it's pretty clear which span is the first and which span is the second.

I recommend a reconsideration but it's not a blocker.

[isaacsu]

The main benefit I have in mind for recording the checkout.attempt field is the convenience of querying these spans where there have been retries. I can remove if enumerating/aggregating sibling spans is easy enough.

[zhming0]

[Not blocking, a question] Am I right that the main intention of this abstraction is so people won't need to pass in e.TracingBackend?

[zhming0]

In another word, do we want to consider making repo name a parameter in this helper? so it can set a consistent attribute?

[isaacsu]

For me, it was a choice between

1. Inline StartSpanFromContext

mirrorSpan, mirrorCtx := tracetools.StartSpanFromContext(ctx, "git.mirror.update", e.TracingBackend)
mirrorSpan.AddAttributes(…)

mirrorDir, err = e.getOrUpdateMirrorDir(mirrorCtx, e.Repository)

mirrorSpan.FinishWithError(err)

2. e.traceOpSpan

mirrorSpan, mirrorCtx := e.traceOpSpan(ctx, "git.mirror.update")
mirrorSpan.AddAttributes(…)

mirrorDir, err = e.getOrUpdateMirrorDir(mirrorCtx, e.Repository)

mirrorSpan.FinishWithError(err)

I chose (2) so that e.TracingBackend can be implied. It returns a plain old Span so I think it's okay for the caller to manage the Finish part without an equivalent abstraction.

What do you think? What benefit would an abstraction for Finishing the span give us?

Aside, might the traceOpSpan name be confusing? Perhaps you have a better name in mind?

[zhming0]

I have doubts in various minor details but they aren't blocking nor one-way door so LGTM 👍