Skip to content

tracer: request Headers as tags for web integrations#1764

Merged
katiehockman merged 85 commits into
mainfrom
mtoff/trace_header_tags
Jun 26, 2023
Merged

tracer: request Headers as tags for web integrations#1764
katiehockman merged 85 commits into
mainfrom
mtoff/trace_header_tags

Conversation

@mtoffl01

@mtoffl01 mtoffl01 commented Feb 24, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes it possible to set request headers as tags at the integration level and at the global tracer level.

Motivation

Feature parity between tracers

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.

@mtoffl01
mtoffl01 requested a review from a team February 24, 2023 18:58
@mtoffl01
mtoffl01 requested a review from a team as a code owner February 24, 2023 18:58
@pr-commenter

pr-commenter Bot commented Feb 27, 2023

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2023-06-14 20:16:17

Comparing candidate commit 2832549 in PR branch mtoff/trace_header_tags with baseline commit 6a62d1f in branch main.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 23 metrics, 0 unstable metrics.

scenario:BenchmarkConcurrentTracing-24

  • 🟩 execution_time [-0.315ms; -0.300ms] or [-23.521%; -22.388%]

Comment thread contrib/go-chi/chi.v5/chi.go Outdated
Comment thread ddtrace/tracer/util_test.go Outdated
Comment thread contrib/net/http/http_test.go
Comment thread ddtrace/tracer/util.go Outdated

@katiehockman katiehockman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should double check that we don't need to do anything with the Header name normalization which is described in the design doc.

Comment thread contrib/emicklei/go-restful/option.go Outdated
Comment thread contrib/emicklei/go-restful/restful_test.go
Comment thread contrib/go-chi/chi.v5/chi.go Outdated
Comment thread contrib/net/http/trace.go Outdated
Comment thread ddtrace/tracer/util.go Outdated
Comment thread contrib/net/http/trace.go Outdated
Comment thread internal/normalizer/normalizer_test.go Outdated
Comment thread internal/normalizer/normalizer_test.go
Comment thread contrib/gorilla/mux/mux.go Outdated
Comment thread contrib/emicklei/go-restful/option.go Outdated
Comment thread internal/globalconfig/globalconfig.go
Comment thread internal/globalconfig/globalconfig.go

@katiehockman katiehockman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Only thing I'm concerned by is that this changes service names. Maybe there was a bad rebase or something

Comment thread contrib/gorilla/mux/option.go Outdated
Comment thread contrib/net/http/option.go Outdated
Comment thread ddtrace/tracer/context.go
Comment thread internal/globalconfig/globalconfig_test.go Outdated
katiehockman
katiehockman previously approved these changes Jun 15, 2023

@katiehockman katiehockman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from that last comment

Comment thread contrib/net/http/option.go Outdated
Comment thread internal/normalizer/normalizer.go
@katiehockman
katiehockman merged commit 1e3868b into main Jun 26, 2023
@katiehockman
katiehockman deleted the mtoff/trace_header_tags branch June 26, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants