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-2720] Remove automatic agentless support #1590

Merged
merged 6 commits into from
Jul 14, 2021

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jul 13, 2021

The profiler supports reporting profiling data directly to the datadog backend, without having to go through an agent. This setup is actively discouraged by the profiling team for users, but hey, it is really handy for testing and prototyping.

But we don't want users to accidentally enable this mode. Up until now, profiler would implicitly use this mode if users set a DD_API_KEY and a DD_SITE configuration. Now, this mode will only be enabled if a DD_PROFILING_AGENTLESS environment variable is set to true.

Since the integration environment does not have a `DD_API_KEY` usually
set, these tests were actually never running.

I've fixed the tests, as well as refactored them to:
* Make sure that transport construction is tested, even if then later
  the test doesn't run (to allow us to catch issues in transport
  creation earlier)
* Run the reporting to agent test even when the agentless test can't
  run. Reporting to agent is our primary use case, so we definitely
  should test it! Agentless is mostly irrelevant, so it's ok if it's
  not running in the default setup for the integration environment.
The `agent_settings` is allowed to be `nil` whenever the agentless
configuration is used, but let's make it clearer that the
default/expected path is not to use agentless, and force callers to
pass in `agent_settings: nil`, rather than using that as a default.

I've also refactored the tests to reflect that this is the primary
usecase.
The profiler supports reporting profiling data directly to the
datadog backend, without having to go through an agent. This setup is
actively discouraged by the profiling team for users, but hey, it is
really handy for testing and prototyping.

But we don't want users to accidentally enable this mode. Up until now,
profiler would implicitly use this mode if users set a `DD_API_KEY` and
a `DD_SITE` configuration. Now, this mode will only be enabled if
a `DD_PROFILING_AGENTLESS` environment variable is set to `true`.
@ivoanjo ivoanjo requested a review from a team July 13, 2021 08:38
@@ -4,6 +4,8 @@ module Core
module Environment
# Defines helper methods for environment
module VariableHelpers
extend self
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: This enables use of the module helpers without needing to include the module.

This test is a little too integration-y: we already have quite a few
other tests that validate agentless mode, so no need to have this one
as well.
Oops! We were actually using a really legacy agent for CI testing,
which did not match the version in docker-compose.yml.

This explains why a profiler integration test was failing in CircleCI
but not in local testing: in CircleCI we were using an agent version
so old that it did not have profiling support!
(We don't yet support profiling JRuby)
@codecov-commenter
Copy link

Codecov Report

Merging #1590 (90262f0) into master (a54750f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1590   +/-   ##
=======================================
  Coverage   98.26%   98.27%           
=======================================
  Files         896      896           
  Lines       43010    43024   +14     
=======================================
+ Hits        42263    42281   +18     
+ Misses        747      743    -4     
Impacted Files Coverage Δ
spec/ddtrace/configuration/components_spec.rb 99.79% <ø> (-0.01%) ⬇️
...ng/transport/http/adapters/net_integration_spec.rb 100.00% <ø> (ø)
lib/datadog/core/environment/variable_helpers.rb 100.00% <100.00%> (ø)
lib/ddtrace/ext/profiling.rb 100.00% <100.00%> (ø)
lib/ddtrace/profiling/transport/http.rb 100.00% <100.00%> (+2.12%) ⬆️
...trace/profiling/transport/http/integration_spec.rb 100.00% <100.00%> (+8.00%) ⬆️
spec/ddtrace/profiling/transport/http_spec.rb 100.00% <100.00%> (ø)
spec/ddtrace/profiling/ext/forking_spec.rb 100.00% <0.00%> (+0.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a54750f...90262f0. Read the comment docs.

@ivoanjo ivoanjo merged commit 0ef7c5f into master Jul 14, 2021
@ivoanjo ivoanjo deleted the ivoanjo/add-flag-for-agentless branch July 14, 2021 07:50
@github-actions github-actions bot added this to the 0.52.0 milestone Jul 14, 2021
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.

None yet

3 participants