feat(contrib): Add rs/zerolog ddtracer integration#4370
feat(contrib): Add rs/zerolog ddtracer integration#4370darccio merged 17 commits intoDataDog:mainfrom
Conversation
|
@darccio I don't have permission to request review from dd-trace-go-guild on this PR - can you add them for me? |
|
@takanuva15 It was automatically requested. 
|
|
@darccio I see some checks are failing but the logs all seem to be wiped out. Can you help clarify what needs to be fixed? 
|
|
@darccio Hi, I see some checks are failing but the logs all seem to be wiped out. Can you help clarify what needs to be fixed? |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdf80c8a04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if ok, _ := regexp.Match(`\d+`, data["dd.trace_id"].([]byte)); !ok { | ||
| t.Errorf("no trace ID") | ||
| } | ||
| if ok, _ := regexp.Match(`\d+`, data["dd.span_id"].([]byte)); !ok { |
There was a problem hiding this comment.
json.Unmarshal into map[string]interface{} yields string values for JSON strings, so data["dd.trace_id"].([]byte) and data["dd.span_id"].([]byte) will panic on the first run of this integration test (the zerolog output encodes these fields as JSON strings). This makes the test fail even when trace/span IDs are present. Use regexp.MatchString on a string value (or cast to string first) to avoid the panic and correctly validate the fields.
Useful? React with 👍 / 👎.
|
@takanuva15 Let me check. |
darccio
left a comment
darccio
left a comment
There was a problem hiding this comment.
You must update ddtrace/tracer/option_test.go test TestAgentIntegration:
func TestAgentIntegration(t *testing.T) {
t.Run("err", func(t *testing.T) {
assert.False(t, MarkIntegrationImported("this-integration-does-not-exist"))
})
// this test is run before configuring integrations and after: ensures we clean up global state
defaultUninstrumentedTest := func(t *testing.T) {
cfg, err := newTestConfig()
assert.Nil(t, err)
defer clearIntegrationsForTests()
cfg.loadContribIntegrations(nil)
assert.Equal(t, 55, len(cfg.integrations)) // <-- it must be 56
for integrationName, v := range cfg.integrations {
assert.False(t, v.Instrumented, "integrationName=%s", integrationName)
}
}
// ...
|
BTW @takanuva15, please run |
|
@takanuva15 Sorry for taking over, but this is faster than asking for changes. I want to merge it this week before the beginning of our next release train. |
lol no problemo, whatever it takes to make it successful |
|
@takanuva15 Can you merge |
# Conflicts: # go.work.sum
done. (I rebased the entire branch onto latest main) |
darccio
left a comment
darccio
left a comment
There was a problem hiding this comment.
Requesting changes to apply linting fixes.
|
@darccio awesome thanks! Do you know which release version this PR will become available? |
2 participants
What does this PR do?
Adds integration for rs/zerolog as discussed here
Motivation
We use zerolog and Datadog together and would like to have Datadog support rs/zerolog for us natively
Reviewer's Checklist
System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.There is a benchmark for any new code, or changes to existing code.If this interacts with the agent in a new way, a system test has been added../scripts/lint.shlocally.Unsure? Have a question? Request a review!