Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename conflicting send_payload method #2673

Merged
merged 1 commit into from
Mar 8, 2023
Merged

Conversation

lloeki
Copy link
Contributor

@lloeki lloeki commented Mar 7, 2023

What does this PR do?

Move a generically named method out of the way.

Motivation

The transport infrastructure is mostly generic, except for a few choke points. This addresses one of them.

The send_payload method, while defined in a trace-specific module, is included at the class level.

When attempting to reuse transport for other use cases, the method conflicts with other modules extending the generic client class.

Luckily its use is very limited and self-contained to a trace transport specific scope. By renaming it additional modules are able to be included and can follow a consistent pattern without conflict.

Additional Notes

This has been discovered while implementing additional transport facilities for remote configuration, from an AppSec impetus.

The whole transport code is fairly involved and due for a rewrite, nonetheless it handles many corner cases. which would make a drop-in rewrite carry much risk to tracing.

Therefore a more segregated approach is being taken to implement the required additional transports, aiming:

  • to be a second use case, highlighting the limitations and hopefully providing insights for improvements.
  • to be of little invasiveness to the current transport code; still, some are required.

This is one of those changes.

How to test the change?

CI should be green.

The transport infrastructure is mostly generic, except for a few choke
points. This addresses one of them.

The `send_payload` method, while defined in a trace-specific module, is
included at the class level.

When attempting to reuse transport for other use cases, the method
conflicts with other modules extending the generic client class.

Luckily its use is very limited and self-contained to a trace transport
specific scope. By renaming it additional modules are able to be
included and can follow a consistent pattern without conflict.
@lloeki lloeki requested a review from a team March 7, 2023 22:18
@codecov-commenter
Copy link

Codecov Report

Merging #2673 (9291a88) into master (5dfea5e) will increase coverage by 0.00%.
The diff coverage is 91.66%.

@@           Coverage Diff           @@
##           master    #2673   +/-   ##
=======================================
  Coverage   98.09%   98.09%           
=======================================
  Files        1168     1168           
  Lines       64210    64212    +2     
  Branches     2857     2857           
=======================================
+ Hits        62986    62991    +5     
+ Misses       1224     1221    -3     
Impacted Files Coverage Δ
spec/ddtrace/transport/traces_spec.rb 97.42% <87.50%> (ø)
lib/ddtrace/transport/http/traces.rb 97.22% <100.00%> (ø)
lib/ddtrace/transport/traces.rb 97.82% <100.00%> (ø)
spec/ddtrace/transport/http/traces_spec.rb 100.00% <100.00%> (ø)
spec/datadog/profiling/native_extension_spec.rb 98.72% <0.00%> (+0.63%) ⬆️
lib/datadog/core/diagnostics/environment_logger.rb 99.24% <0.00%> (+1.55%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@GustavoCaso GustavoCaso left a comment

Choose a reason for hiding this comment

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

The change LGTM.

Regarding your comment:

When attempting to reuse transport for other use cases, the method conflicts with other modules extending the generic client class.

Would be interesting to provide an example 😄

@lloeki lloeki merged commit e3cdbb4 into master Mar 8, 2023
@lloeki lloeki deleted the isolate-trace-transport branch March 8, 2023 13:46
@github-actions github-actions bot added this to the 1.11.0 milestone Mar 8, 2023
@lloeki lloeki modified the milestones: 1.11.0, 1.10.1 Mar 10, 2023
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.

3 participants