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

[PROF-6070] Remove legacy profiling transport #2062

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jun 1, 2022

In #2059 we changed the profiler to use the new HttpTransport to report profiling data, but we left around the legacy transport code:

Afterwards, I got to thinking that if customers do report any issue that we suspect may be related to HttpTransport, it would be really great if there was an easy way to test that out by simply toggling a configuration option, rather than customers needing to compare different versions of dd-trace-rb. So, I've revived the old code, but changed it to comply to the new interfaces and structures. This makes it trivial to remove it again: just revert the Retrofit legacy profiling transport implementation as a backup to HttpTransport class and Allow enabling legacy profiling transport codepath via setting/env variable commits.

The 1.2.0->1.5.0 releases have defaulted to use the new Profiling::HttpTransport, and other than #2151/#2152 which was solved in 1.3.0, no other issues have been reported so I think we're ready to remove the old code.

@ivoanjo ivoanjo self-assigned this Jul 13, 2022
@ivoanjo ivoanjo added the profiling Involves Datadog profiling label Jul 13, 2022
…p to `HttpTransport` class"

This reverts commit 3b7126d.

This has been replaced by the `HttpTransport` class, and was only left
behind as a temporary backup in case there were any issues in
`HttpTransport`.
…g/env variable"

This reverts commit 09d83b3.

This has been replaced by the `HttpTransport` class, and was only left
behind as a temporary backup in case there were any issues in
`HttpTransport`.
@ivoanjo ivoanjo force-pushed the ivoanjo/remove-legacy-profiling-transport branch from 6d66a36 to b19b8fb Compare October 3, 2022 08:25
@ivoanjo ivoanjo changed the title (Merge only after September 2022) Remove legacy profiling transport [PROF-6070] Remove legacy profiling transport Oct 3, 2022
@ivoanjo ivoanjo marked this pull request as ready for review October 3, 2022 08:44
@ivoanjo ivoanjo requested a review from a team October 3, 2022 08:44
@ivoanjo
Copy link
Member Author

ivoanjo commented Oct 4, 2022

Thanks Marco for the review! As discussed via slack, I'm going to hold off on merging this until next week :)

@ivoanjo ivoanjo merged commit f14855b into master Oct 7, 2022
@ivoanjo ivoanjo deleted the ivoanjo/remove-legacy-profiling-transport branch October 7, 2022 08:39
@github-actions github-actions bot added this to the 1.6.0 milestone Oct 7, 2022
@TonyCTHsu TonyCTHsu mentioned this pull request Nov 15, 2022
ivoanjo added a commit that referenced this pull request Feb 16, 2023
**What does this PR do?**:

This PR removes a `merge` method that is no longer in use. This is
safe to remove because it is not part of ddtrace's public API.

This was only used by the legacy profiling transport (as hinted in
the comment attached to the code), and the legacy profiling
transport was removed in
#2062 (search for
`configure_for_agent` inside `lib/datadog/profiling/transport/http.rb`).

**Motivation**:

Remove unused code :)

**Additional Notes**:

N/A

**How to test the change?**:

Check that CI is still green.
ivoanjo added a commit that referenced this pull request Feb 16, 2023
**What does this PR do?**:

This PR removes a `merge` method that is no longer in use. This is
safe to remove because it is not part of ddtrace's public API.

This was only used by the legacy profiling transport (as hinted in
the comment attached to the code), and the legacy profiling
transport was removed in
#2062 (search for
`configure_for_agent` inside `lib/datadog/profiling/transport/http.rb`).

**Motivation**:

Remove unused code :)

**Additional Notes**:

N/A

**How to test the change?**:

Check that CI is still green.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants