feat(contrib/zap): add log/trace correlation for uber-go/zap#4610
feat(contrib/zap): add log/trace correlation for uber-go/zap#4610highlyavailable wants to merge 15 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@highlyavailable Thanks for contributing it! It'd be great if you can add an |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
|
@highlyavailable Check the CI checks, as there are some failing. |
Add PackageGoUberOrgZap constant and PackageInfo entry to prepare for the upcoming uber-go/zap logging contrib integration.
Add contrib/go.uber.org/zap with a TraceFields helper that extracts
Datadog trace and span IDs from context and returns them as zap fields.
Supports both 128-bit and 64-bit trace ID formats.
Usage: logger.Info("msg", ddzap.TraceFields(ctx)...)
Add go.uber.org/zap to the contribIntegrations map in tracer/option.go so it shows up in startup diagnostics. Also add the package to supported_integrations.md and update go.work.sum.
Adds an orchestrion.yml that marks go.uber.org/zap as imported when Orchestrion auto-instrumentation is active. Full automatic context injection isn't possible since zap's zapcore.Core.Write() doesn't accept context.Context, but integration discovery is handled.
9895fa7 to
6a25ead
Compare
Hi @darccio, I added a commit with an
Made some updates for the failing CI checks, waiting on the approval now 👍 |
|
@highlyavailable Thanks. I'll let @DataDog/apm-idm-go to discuss the implementation design. About the |
Was failing CI jobs, after reading through their logs I think I need to set the integration count (56 → 57) and register DD_TRACE_ZAP_ANALYTICS_ENABLED as a supported config
|
Hi @darccio. Just pushed an update to tests and config which should take care of the failing CI. For orchestrion.yml, what’s in there right now is just wiring up integration telemetry. I wasn’t really sure about the best way to handle actual trace injection given zap’s constraints, so would really appreciate any guidance on how you (or others) all think that should be done. Also saw the overlap with #4241, main difference is I went with That said, I’m very open to aligning with #4241 or reshaping the API if you have a preferred direction, happy to adjust and thank you for the feedback so far |
…g-correlation # Conflicts: # go.work.sum
|
@highlyavailable I'll check this again soon. |
…o v0.15.0 from feedback from failing CI check-modules
highlyavailable
changed the title
|
@darccio sorry it looks like I missed a few static CI checks, I fixed the PR title and updated indirect dependencies in |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Hey @darccio, I believe the generate job is timing out because my I am not sure about the right way to handle that... should I run ./scripts/generate.sh locally and commit the orchestrion/all changes? Or is there something specific expected there for a new integration? |
|
@highlyavailable Yes, you should run All the details are explained in our CONTRIBUTING.md, although I noticed that we don't explain |
|
Hey @darccio. The 2 red CI checks are System Tests and all-jobs-are-green, and I'm pretty sure they're just a fork-PR artifact rather than anything with the diff. The System Tests matrix jobs get skipped on fork PRs (the gate around system-tests.yml:58 requires head.repo.full_name == 'DataDog/dd-trace-go'), but the system-tests-done aggregator at the bottom of the file treats skipped as failure, so it always goes red. Looking at the ruleset though, the actual required check is devflow/mergegate, and that one's been pending on this PR for a while. It passed cleanly on #4657 (a similar fork PR) even though System Tests was red there too, so I don't think the red checks are the real blocker. Any chance you could give devflow a nudge, or let me know if there's something I should do on my end to get it to evaluate? Happy to send a separate PR against the workflow to make the aggregator treat skipped as success |
|
@highlyavailable I'm going to check this tomorrow. |
This comment has been minimized.
This comment has been minimized.
|
Closing this PR as we are continuing the work in this PR: #4729 to have a zap integration with Orchestrion support. Thanks @highlyavailable for your contribution! |
3 participants
What does this PR do?
Adds a new contrib package at
contrib/go.uber.org/zapthat provides log/trace correlation foruber-go/zap. Just the single helperTraceFields(ctx context.Context) []zap.Fieldwhich pulls DD trace and span IDs from the context and returns them as zap fields.Usage:
Zap's
zapcore.Core.Write()doesn't takecontext.Context, wrapping the core didn't seem like an option, but I am happy to get your feedback if you can think of a better implementation.I added support for both 128-bit and 64-bit trace ID formats via
DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED.Motivation
go.uber.org/zapis one of the most widely used structured loggers in Go and there's currently no Datadog logging integration for it. This closes that gap, following the same patterns used by the existing zerolog and logrus contribs.