feat(contrib/net/http): add WithClientTimings option#3839
Conversation
…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]>
ad2f226 to
e0fff1c
Compare
darccio
left a comment
There was a problem hiding this comment.
LGTM. Additional allocation is reasonable for an opt-in feature.
|
@darccio |
|
@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
left a comment
There was a problem hiding this comment.
Thanks! this looks great - just requested a minor change on the option name to better reflect what it does 🙏
| // 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 { |
There was a problem hiding this comment.
| 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).
There was a problem hiding this comment.
@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.
|
/merge |
|
View all feedbacks in Devflow UI.
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.
The expected merge time in
|
Co-authored-by: rarguelloF <[email protected]> Co-authored-by: darccio <[email protected]>
Summary
Adds support for
httptrace.ClientTraceon wrappedhttp.Clientrequests.When enabled via
WithClientTimings(true), captures detailed HTTP request timing as span tags for debugging network performance issues.Changes
WithClientTimings(bool)option (disabled by default)Span Tags Added
http.dns.duration_ms,http.connect.duration_ms,http.tls.duration_mshttp.get_conn.duration_ms,http.first_byte.duration_mshttp.connect.error,http.tls.error(when errors occur)Usage
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.Poolfor the timing struct, or is the current simple allocation acceptable given the debugging use case?Fixes #1598