Skip to content

Transports: drop broken super call in InternalErrorResponse#to_s#5762

Open
p-datadog wants to merge 4 commits into
masterfrom
ddsign/rc-transport-error-inspect
Open

Transports: drop broken super call in InternalErrorResponse#to_s#5762
p-datadog wants to merge 4 commits into
masterfrom
ddsign/rc-transport-error-inspect

Conversation

@p-datadog
Copy link
Copy Markdown
Member

@p-datadog p-datadog commented May 13, 2026

What does this PR do?
Replaces the super call in Datadog::Core::Transport::InternalErrorResponse#to_s with self.class, so the prefix of the error message is the class name instead of #<…:0xADDR>.

Motivation:
Reported in https://github.com/DataDog/ruby-guild/issues/241 (originally surfaced via APMS-15827).

InternalErrorResponse includes the Response module and defines both to_s and inspect with the same body — "#{super}, error_type:#{error.class} error:#{error}" — apparently assuming the module defines both methods symmetrically. It doesn't: Response defines inspect only.

Method resolution order is InternalErrorResponseResponse (module) → Object. inspect's super finds Response#inspect and returns the predicates-and-payload string. to_s's super skips Response (no to_s there) and lands on Object#to_s, whose default implementation returns "#<ClassName:0x<hex address>>". So the same super pattern works in one method and silently emits a memory address in the other.

The trailing error_type:<klass> error:<message> portion is already useful (PR #4669 wraps agent HTTP errors in AgentErrorResponse whose message includes the response code and truncated body), so this PR only fixes the leading garbage. The TransportError raise site in Datadog::Core::Remote::Client#sync is unchanged — it still calls response.to_s, but to_s now produces a clean string.

Change log entry
Yes. Remote configuration TransportError messages no longer include the wrapper response object's memory address.

Additional Notes:
Before / after (Datadog::Core::Remote::Client::TransportError#message for the two internal_error? paths):

Network error (IOError):

Before:

#<Datadog::Core::Transport::InternalErrorResponse:0x00007c10a998bf30>, error_type:IOError error:Connection reset by peer

After:

Datadog::Core::Transport::InternalErrorResponse, error_type:IOError error:Connection reset by peer

Agent HTTP 500:

Before:

#<Datadog::Core::Transport::InternalErrorResponse:0x00007c10a998b828>, error_type:Datadog::Core::Remote::Transport::HTTP::Config::Response::AgentErrorResponse error:Agent returned an error response: 500: rpc error: code = DeadlineExceeded desc = context deadline exceeded

After:

Datadog::Core::Transport::InternalErrorResponse, error_type:Datadog::Core::Remote::Transport::HTTP::Config::Response::AgentErrorResponse error:Agent returned an error response: 500: rpc error: code = DeadlineExceeded desc = context deadline exceeded

How to test the change?
Two new assertions in spec/datadog/core/transport/response_spec.rb pin the new behavior: #to_s must not contain 0x\h+ and must start with Datadog::Core::Transport::InternalErrorResponse,. The pre-existing /StandardError/ assertion still passes.

The TransportError raised from Datadog::Core::Remote::Client#sync used
response.to_s as the message. Neither Datadog::Core::Transport::Response
nor Datadog::Core::Transport::HTTP::Response defines to_s, and
InternalErrorResponse#to_s calls super, which falls through to
Object#to_s and produces "#<...:0xADDR>" with no useful information.

Switching the raise site to response.inspect resolves super to
Response#inspect, which already emits the predicates, optional HTTP
code, and truncated payload — and InternalErrorResponse#inspect appends
the wrapped error_type and error message.

Reported in https://github.com/DataDog/ruby-guild/issues/241.
@p-datadog p-datadog requested a review from a team as a code owner May 13, 2026 21:18
@p-datadog p-datadog added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label May 13, 2026
@dd-octo-sts dd-octo-sts Bot added the core Involves Datadog core libraries label May 13, 2026
Strengthen the existing assertions in client_spec.rb so they fail if
the raise site reverts to response.to_s. The internal_error?:true
prefix is emitted by Response#inspect; with response.to_s the message
would start with #<...:0xADDR> and the prefix would be absent.
@datadog-prod-us1-4
Copy link
Copy Markdown

datadog-prod-us1-4 Bot commented May 13, 2026

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 97.16% (+0.01%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 958cda0 | Docs | Datadog PR Page | Give us feedback!

p-ddsign added 2 commits May 13, 2026 19:06
Revert "RC: include response details in TransportError message"

Switching approach: fix InternalErrorResponse#to_s directly instead of
changing the raise site. The previous approach only swapped an
"#<...:0xADDR>" prefix for a string of mostly-nil predicates without
adding signal beyond what PR #4669 already produces.
The Response module does not define to_s, so the super call in
InternalErrorResponse#to_s resolves to Object#to_s and emits
"#<Datadog::Core::Transport::InternalErrorResponse:0xADDR>" as the
prefix of the error message — replacing the wrapped exception info that
follows with a memory address.

Replace super with self.class so the prefix is the class name rather
than the default object identifier. The trailing
"error_type:<klass> error:<message>" portion (already useful since
PR #4669 added AgentErrorResponse with an informative message) is
unchanged.

Reported in https://github.com/DataDog/ruby-guild/issues/241.
@p-datadog p-datadog changed the title RC: include response details in TransportError message Transports: drop broken super call in InternalErrorResponse#to_s May 13, 2026
@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented May 13, 2026

Benchmarks

Benchmark execution time: 2026-05-13 23:33:13

Comparing candidate commit 958cda0 in PR branch ddsign/rc-transport-error-inspect with baseline commit 21ea64b in branch master.

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

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos core Involves Datadog core libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants