Skip to content

feat(contrib/net/http): add WithClientTimings option#3839

Merged
dd-mergequeue[bot] merged 4 commits into
DataDog:mainfrom
korECM:feature/http-client-trace
Sep 5, 2025
Merged

feat(contrib/net/http): add WithClientTimings option#3839
dd-mergequeue[bot] merged 4 commits into
DataDog:mainfrom
korECM:feature/http-client-trace

Conversation

@korECM

@korECM korECM commented Aug 2, 2025

Copy link
Copy Markdown
Contributor

Summary

Adds support for httptrace.ClientTrace on wrapped http.Client requests.

When enabled via WithClientTimings(true), captures detailed HTTP request timing as span tags for debugging network performance issues.

Changes

  • Added WithClientTimings(bool) option (disabled by default)
  • Captures timing for DNS, connection, TLS, connection pool, and first byte
  • Includes error information for connection and TLS failures

Span Tags Added

  • http.dns.duration_ms, http.connect.duration_ms, http.tls.duration_ms
  • http.get_conn.duration_ms, http.first_byte.duration_ms
  • http.connect.error, http.tls.error (when errors occur)

Usage

client := &http.Client{
    Transport: httptrace.WrapRoundTripper(http.DefaultTransport,
        httptrace.WithClientTimings(true),
    ),
}

Performance

Opt-in design ensures zero overhead when disabled (default). When enabled, allocates a small struct per request.

Question for reviewers: Would you prefer using sync.Pool for the timing struct, or is the current simple allocation acceptable given the debugging use case?

Fixes #1598

…tp.Client requests

When enabled via WithClientTrace(true), the integration captures detailed timing
information for DNS lookups, connection establishment, TLS handshakes, and other
HTTP request events as span tags. This provides valuable insights for debugging
network performance issues and connection problems.

Added span tags:
- http.dns.duration_ms: DNS resolution timing
- http.connect.duration_ms: TCP connection establishment timing
- http.tls.duration_ms: TLS handshake timing
- http.get_conn.duration_ms: Connection pool acquisition timing
- http.first_byte.duration_ms: Time to first response byte
- http.connect.error: Connection failure details (when errors occur)
- http.tls.error: TLS handshake failure details (when errors occur)

This enables developers to:
- Distinguish between DNS vs TLS vs connection bottlenecks
- Verify connection pooling is working as expected
- Debug slow network requests with precise timing breakdowns
- Identify specific failure points in the HTTP request lifecycle

The feature is opt-in (disabled by default) to avoid performance overhead
for users who don't need detailed timing information.

Fixes DataDog#1598

Signed-off-by: Jongwon Youn <[email protected]>
@korECM
korECM force-pushed the feature/http-client-trace branch from ad2f226 to e0fff1c Compare August 23, 2025 11:58

@darccio darccio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Additional allocation is reasonable for an opt-in feature.

@korECM

korECM commented Sep 3, 2025

Copy link
Copy Markdown
Contributor Author

@darccio
Hi, could you please provide information about the release schedule for this feature? Thank you!

@darccio darccio changed the title contrib/net/http: add support for httptrace.ClientTrace on wrapped http.Client requests feat(contrib/net/http(: add support for httptrace.ClientTrace on wrapped http.Client requests Sep 4, 2025
@darccio darccio changed the title feat(contrib/net/http(: add support for httptrace.ClientTrace on wrapped http.Client requests feat(contrib/net/http): add support for httptrace.ClientTrace on wrapped http.Client requests Sep 4, 2025
@darccio

darccio commented Sep 4, 2025

Copy link
Copy Markdown
Member

@korECM This PR is still waiting for @DataDog/apm-idm-go approval. Let me ping @rarguelloF. I also fixed the PR title to follow our new convention based on conventional commits.

@rarguelloF rarguelloF 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.

Thanks! this looks great - just requested a minor change on the option name to better reflect what it does 🙏

Comment thread contrib/net/http/option.go Outdated
// When enabled, the integration will add timing information for DNS lookups,
// connection establishment, TLS handshakes, and other HTTP request events as span tags.
// This feature is disabled by default and adds minimal overhead when enabled.
func WithClientTrace(enabled bool) RoundTripperOptionFn {

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.

Suggested change
func WithClientTrace(enabled bool) RoundTripperOptionFn {
func WithClientTimings(enabled bool) RoundTripperOptionFn {

I think this name will make more clear what the option does, since what the package currently does is tracing as well (this option adds additional timing tags).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rarguelloF
I've updated all occurrences of WithClientTrace to WithClientTimings throughout the codebase. The changes have been pushed and are ready for your review.
d5c5caa (#3839)

As requested in PR review, renamed the function from WithClientTrace to
WithClientTimings to better reflect that this option adds timing information
tags rather than just enabling tracing.
@korECM
korECM requested a review from rarguelloF September 5, 2025 02:00
@rarguelloF rarguelloF changed the title feat(contrib/net/http): add support for httptrace.ClientTrace on wrapped http.Client requests feat(contrib/net/http): add WithClientTimings option Sep 5, 2025
@rarguelloF

Copy link
Copy Markdown
Contributor

/merge

@dd-devflow-routing-codex

dd-devflow-routing-codex Bot commented Sep 5, 2025

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2025-09-05 09:02:38 UTC ℹ️ Start processing command /merge


2025-09-05 09:02:47 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2025-09-05 11:01:07 UTC ℹ️ MergeQueue: merge request added to the queue

The expected merge time in main is approximately 15m (p90).


2025-09-05 11:13:58 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue
dd-mergequeue Bot merged commit ef95cbd into DataDog:main Sep 5, 2025
149 of 159 checks passed
kakkoyun pushed a commit to kakkoyun/dd-trace-go that referenced this pull request Sep 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for httptrace.ClientTrace on wrapped http.Client requests

3 participants