Merge pull request #868 from cloudfoundry/tylerbenson-datadog-javaagent

Polishing for PR #867

Author: Daniel Mikusa

.gitignore

@@ -5,3 +5,4 @@
 .yardoc
 build/
 doc
+.envrc

docs/framework-datadog_javaagent.md

@@ -1,18 +1,23 @@
 # Datadog APM Javaagent Framework
-The [Datadog APM]() Javaagent Framework allows your application to be dynamically instrumented [by][datadog-javaagent] `dd-java-agent.jar`.
+The [Datadog APM]() Javaagent Framework installs an agent that allows your application to be dynamically instrumented [by][datadog-javaagent] `dd-java-agent.jar`. 
+
+For this functionality to work, you **must** also use this feature in combination with the [Datadog Cloudfoundry Buildpack](). The Datadog Cloudfoundry Buildpack **must** run first, so that it can supply the components to which the Datadog APM agent will talk. Please make sure you follow the instructions on the README for the Datadog Cloudfoundry Buildpack to enable and configure it.
+
+The framework will configure the Datadog agent for correct use in most situations, however you may adjust its behavior by setting additional environment variables. For a complete list of Datadog Agent configuration options, please see the [Datadog Documentation](https://docs.datadoghq.com/tracing/setup_overview/setup/java/?tab=containers#configuration).
 
 <table>
   <tr>
-    <td><strong>Detection Criterion</strong></td><td>One of the following environment variables configured:
-      <ul>
-        <li><code>DD_APM_ENABLED</code> configured as <code>true</code></li>
-        <li><code>DD_API_KEY</code> defined with the assumption of a <a href='https://github.com/DataDog/datadog-cloudfoundry-buildpack'>datadog-cloudfoundry-buildpack</a> configured, and <code>DD_APM_ENABLED</code> not <code>false</code></li>
-      </ul>
+    <td><strong>Detection Criterion</strong></td><td>All must be true:
+        <ul>
+            <li>The Datadog Buildpack must be included</li>
+            <li><code>DD_API_KEY</code> defined and contain your API key</li>
+        </ul>
+        Optionally, you may set <code>DD_APM_ENABLED</code> to <code>false</code> to force the framework to not contribute the agent.
     </td>
   </tr>
   <tr>
     <td><strong>Tags</strong></td>
-    <td><tt>datadog-javaagent=&lt;version&gt;</tt></td>
+    <td><code>datadog-javaagent=&lt;version&gt;</code></td>
   </tr>
 </table>
 
@@ -33,6 +38,7 @@ The javaagent can be configured directly via environment variables or system pro
 
 [Configuration and Extension]: ../README.md#configuration-and-extension
 [Datadog APM]: https://www.datadoghq.com/product/apm/
+[Datadog Cloudfoundry Builpack]: https://github.com/DataDog/datadog-cloudfoundry-buildpack
 [datadog-javaagent]: https://github.com/datadog/dd-trace-java
 [Configuration of Datadog Javaagent]: https://docs.datadoghq.com/tracing/setup_overview/setup/java/#configuration
 [this listing]: https://raw.githubusercontent.com/datadog/dd-trace-java/cloudfoundry/index.yml

lib/java_buildpack/framework/datadog_javaagent.rb

@@ -25,23 +25,35 @@ module Framework
     class DatadogJavaagent < JavaBuildpack::Component::VersionedDependencyComponent
       include JavaBuildpack::Util
 
+      def initialize(context)
+        super(context)
+        @datadog_buildpack = File.exist? File.join(@droplet.root, 'datadog')
+        @logger = JavaBuildpack::Logging::LoggerFactory.instance.get_logger DatadogJavaagent
+      end
+
       # (see JavaBuildpack::Component::BaseComponent#compile)
       def compile
-        download_jar
+        @logger.error 'Datadog Buildpack is required, but not found' unless @datadog_buildpack
+
+        download_jar if @datadog_buildpack
       end
 
       # (see JavaBuildpack::Component::BaseComponent#release)
       def release
-        java_opts   = @droplet.java_opts
+        return unless @datadog_buildpack
+
+        java_opts = @droplet.java_opts
         java_opts.add_javaagent(@droplet.sandbox + jar_name)
 
-        if !@application.environment.key?('DD_SERVICE')
-          java_opts.add_system_property('dd.service', @application.details['application_name'])
+        unless @application.environment.key?('DD_SERVICE')
+          app_name = @configuration['default_application_name'] || @application.details['application_name']
+          java_opts.add_system_property('dd.service', "\\\"#{app_name}\\\"")
         end
 
-        if @application.details['application_version']
-          java_opts.add_system_property('dd.version', @application.details['application_version'])
-        end
+        return unless @application.details['application_version']
+
+        version = @configuration['default_application_version'] || @application.details['application_version']
+        java_opts.add_system_property('dd.version', version)
       end
 
       protected
@@ -50,8 +62,7 @@ def release
       def supports?
         api_key_defined = @application.environment.key?('DD_API_KEY') && !@application.environment['DD_API_KEY'].empty?
         apm_disabled = @application.environment['DD_APM_ENABLED'] == 'false'
-        apm_enabled = @application.environment['DD_APM_ENABLED'] == 'true'
-        (api_key_defined && !apm_disabled) || apm_enabled
+        (api_key_defined && !apm_disabled)
       end
     end
   end

spec/fixtures/stub-datadog-javaagent.jar

@@ -23,6 +23,11 @@
 describe JavaBuildpack::Framework::DatadogJavaagent do
   include_context 'with component help'
 
+  let(:configuration) do
+    { 'default_application_version' => nil,
+      'default_application_name' => nil }
+  end
+
   describe '#detect' do
     subject(:detect) { component.detect }
 
@@ -45,7 +50,7 @@
     context 'when apm is enabled with no api key' do
       let(:environment) { { 'DD_APM_ENABLED' => 'true' } }
 
-      it { is_expected.to eq("datadog-javaagent=#{version}") }
+      it { is_expected.to be nil }
     end
 
     context 'when apm key is provided' do
@@ -56,16 +61,50 @@
   end
 
   context 'when apm key is provided' do
-    let(:environment) { { 'DD_API_KEY' => 'foo' } }
+    let(:environment) do
+      super().update({ 'DD_API_KEY' => 'foo' })
+    end
 
-    it 'compile downloads datadog-javaagent JAR', cache_fixture: 'stub-datadog-javaagent.jar' do
-      component.compile
-      expect(sandbox + "datadog-javaagent-#{version}.jar").to exist
+    context 'when datadog buildpack is present' do
+      before do
+        FileUtils.mkdir_p File.join(context[:droplet].root, 'datadog')
+      end
+
+      after do
+        FileUtils.rmdir File.join(context[:droplet].root, 'datadog')
+      end
+
+      it 'compile downloads datadog-javaagent JAR', cache_fixture: 'stub-datadog-javaagent.jar' do
+        component.compile
+        expect(sandbox + "datadog_javaagent-#{version}.jar").to exist
+      end
+
+      it 'release updates JAVA_OPTS' do
+        component.release
+
+        expect(java_opts).to include(
+          "-javaagent:$PWD/.java-buildpack/datadog_javaagent/datadog_javaagent-#{version}.jar"
+        )
+        expect(java_opts).to include('-Ddd.service=\"test-application-name\"')
+        expect(java_opts).to include('-Ddd.version=test-application-version')
+      end
     end
 
-    it 'release updates JAVA_OPTS' do
-      component.release
-      expect(java_opts).to include("-javaagent:$PWD/.java-buildpack/datadog_javaagent/datadog_javaagent-#{version}.jar")
+    context 'when datadog buildpack is not present' do
+      it 'compile downloads datadog-javaagent JAR', cache_fixture: 'stub-datadog-javaagent.jar' do
+        component.compile
+        expect(sandbox + "datadog_javaagent-#{version}.jar").not_to exist
+      end
+
+      it 'release updates JAVA_OPTS' do
+        component.release
+
+        expect(java_opts).not_to include(
+          "-javaagent:$PWD/.java-buildpack/datadog_javaagent/datadog_javaagent-#{version}.jar"
+        )
+        expect(java_opts).not_to include('-Ddd.service=\"test-application-name\"')
+        expect(java_opts).not_to include('-Ddd.version=test-application-version')
+      end
     end
   end
 end

spec/java_buildpack/framework/datadog_javaagent_spec.rb

@@ -115,7 +115,7 @@
     context do
 
       let(:environment) do
-        { 'JBP_CONFIG_TEST' => '{version:1.8.+}' }
+        { 'JBP_CONFIG_TEST' => '{version: 1.8.+' }
       end
 
       it 'diagnoses invalid YAML syntax' do