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.

Sign up for GitHub

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

Jump to bottom

Reworked Dynatrace integrations #336

Merged

nebhale merged 1 commit into cloudfoundry:master from aloismayr:master

Oct 10, 2016

Contributor

aloismayr commented Oct 5, 2016

In this PR I've reworked the our integrations and worked on the following:

Renamed Ruxit to Dynatrace OneAgent
Renamed DynaTrace to Dynatrace Appmon
Updated detection criterion for Dynatrace OneAgent (SaaS/Managed)
- tenant, tenanttoken and server are deprecated (but still supported for the sake of backward compatibilty)
- environmentid, apitoken and endpoint are the new required fields
Dynatrace OneAgent will be downloaded from Dynatrace SaaS/Managed cluster via REST API (if apitoken given) rather than from the public repository
Public repository will still be maintained for ensuring backward compatibility
Updated documentation and tests
Minor bugfix to support caching of HTTP downloads with query parameters

cfdreddbot commented Oct 5, 2016

Hey aloismayr!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

nebhale suggested changes

View reviewed changes

Contributor

nebhale left a comment

Another awesome contribution. Most of my requests are either documentation changes or clarification on bits and pieces. Feel free to push back if you think I'm asking for things that aren't appropriate.

config/dynatrace_appmon_agent.yml

@@ @@ -17,4 +17,4 @@ @@
               ---
               version: 6.3.0_+
               repository_root: http://downloads.dynatracesaas.com/cloudfoundry/buildpack/java
-              default_agent_name:

Contributor

nebhale Oct 10, 2016

I know this seems crazy, but the space was here for a reason and should be replaced. I don't believe this will be an issue for you as this change has no net result.

Contributor Author

aloismayr Oct 10, 2016

No clue how and why the space has disappeared. I added it again.

docs/framework-dynatrace_appmon_agent.md

    
              The applications Cloud Foundry name is used as the `agent group` in DynaTrace, and must be pre-configured on the DynaTrace server.

              The applications Cloud Foundry name is used as the `agent group` in Dynatrace Appmon, and must be pre-configured on the Dynatrace server.

Contributor

nebhale Oct 10, 2016

application's

Contributor Author

aloismayr Oct 10, 2016

Thank you. Changed that!

docs/framework-dynatrace_appmon_agent.md

    
              * The DynaTrace agent slows down app execution significantly at first, but gets faster over time. Setting the application manifest to contain `maximum_health_check_timeout` of 180 or more and/or using `cf push -t 180` or more when pushing a DynaTrace-monitored application may help.

              * Multiple `cf push`s will cause dead penguins to build up in the DynaTrace agent dashboard, as CF launches/disposes application containers. These can be hidden but will collect in the dynatrace database.

              * The Dynatrace Appmon agent slows down app execution significantly at first, but gets faster over time. Setting the application manifest to contain `maximum_health_check_timeout` of 180 or more and/or using `cf push -t 180` or more when pushing a Dynatrace-monitored application may help.

              * Multiple `cf push`s will cause dead penguins to build up in the Dynatrace Appmon dashboard, as CF launches/disposes application containers. These can be hidden but will collect in the Dynatrace database.

Contributor

nebhale Oct 10, 2016

Just want to double check that you actually mean "dead penguins" and it's not an auto-correct thing.

Contributor Author

aloismayr Oct 10, 2016

Thank you. Reworked the complete NOTE section.

docs/framework-dynatrace_appmon_agent.md

               <table>
                 <tr>
-                  <td><strong>Detection Criterion</strong></td><td>Existence of a single bound DynaTrace service.
+                  <td><strong>Detection Criterion</strong></td><td>Existence of a single bound Dynatrace service.

Contributor

nebhale Oct 10, 2016

Do we want to call this a "single bound Dynatrace AppMon service" with the same clarity on the other type of Dynatrace service?

Contributor Author

aloismayr Oct 10, 2016

Thank you. Yeah, changed "Dynatrace service" to "Dynatrace Appmon service" where applicable and meaningful.

docs/framework-dynatrace_appmon_agent.md

    
                </tr>

              </table>

              Tags are printed to standard output by the buildpack detect script

              ## User-Provided Service

              Users must provide their own DynaTrace service. A user-provided DynaTrace service must have a name or tag with `dynatrace` in it so that the DynaTrace Agent Framework will automatically configure the application to work with the service.

              Users must provide their own Dynatrace service. A user-provided Dynatrace service must have a name or tag with `dynatrace` in it so that the Dynatrace Appmon Agent Framework will automatically configure the application to work with the service.

Contributor

nebhale Oct 10, 2016

Same clarification of which Dynatrace service here. Specifically, I'm not asking to change the detection criteria, just the documentation of that criteria.

Contributor Author

aloismayr Oct 10, 2016

Thanks, also changed the wording to Dynatrace Appmon service here.

docs/framework-dynatrace_appmon_agent.md

    
                    <ul>

                      <li>Existence of a DynaTrace service is defined as the <a href="http://docs.cloudfoundry.org/devguide/deploy-apps/environment-variable.html#VCAP-SERVICES"><code>VCAP_SERVICES</code></a> payload containing a service who's name, label or tag has <code>dynatrace</code> as a substring and contains <code>server</code> field in the credentials. Note: The credentials must <b>NOT</b> contain <code>tenant</code> and <code>tenanttoken</code> in order to make sure the detection mechanism does not interfere with Dynatrace Ruxit integration.</li>

                      <li>Existence of a Dynatrace service is defined as the <a href="http://docs.cloudfoundry.org/devguide/deploy-apps/environment-variable.html#VCAP-SERVICES"><code>VCAP_SERVICES</code></a> payload containing a service who's name, label or tag has <code>dynatrace</code> as a substring and contains <code>server</code> field in the credentials. Note: The credentials must <b>NOT</b> contain <code>tenant</code> and <code>tenanttoken</code> in order to make sure the detection mechanism does not interfere with Dynatrace SaaS/Managed integration.</li>

Contributor

nebhale Oct 10, 2016

Same clarification here.

Contributor Author

aloismayr Oct 10, 2016

Changed "Dynatrace service" to "Dynatrace Appmon service" where applicable and meaningful.

docs/framework-dynatrace_one_agent.md

+              <table>
+                <tr>
+                  <td><strong>Detection Criterion</strong></td><td>Existence of a single bound Dynatrace service.

Contributor

nebhale Oct 10, 2016

The reverse of the earlier Dynatrace service clarification.

Contributor Author

aloismayr Oct 10, 2016

Thanks! Changed to Dynatrace SaaS/Managed service.

docs/framework-dynatrace_one_agent.md

+                    </ul>
+                  </td>
+                </tr>
+              </table>

Contributor

nebhale Oct 10, 2016

Missing listing of tags.

Contributor Author

aloismayr Oct 10, 2016

Also added the tag row.

lib/java_buildpack/framework/dynatrace_one_agent.rb

+                      download_uri = server(credentials).gsub('/communication', '').gsub(':8443', '').gsub(':443', '')
+                      download_uri += '/api/v1/deployment/installer/agent/unix/paas/latest?include=java&bitness=64&'
+                      download_uri += "Api-Token=#{credentials[APITOKEN]}"
+                      ['latest', download_uri]

Contributor

nebhale Oct 10, 2016

Generally latest is good enough here. Just want to be clear that not having a real version means that it'll be difficult for operators to determine which applications are vulnerable our out of date. It's more a serviceability issue than anything else and I'd encourage you to put in a real version if possible. If not, this is fine.

Contributor Author

aloismayr Oct 10, 2016

Agreed. At the time of compiling the download URL we don't know about the exact version we will get. We only know it is the latest version available for the respective environment. We could add a log-message in the compile or release step tough.

Contributor

nebhale Oct 10, 2016

OK, no worries. Just one of those things to think about for the longer term. Maybe another URI that exposes the version or even a HEAD request to that URI which returns a header with the value in it. Nothing that can't be added in the future.

Contributor Author

aloismayr Oct 10, 2016

Understand, sounds good to me.

lib/java_buildpack/util/cache/cached_file.rb

@@ @@ -33,7 +33,7 @@ class CachedFile @@
                       # @param [String] uri a uri which uniquely identifies the file in the cache
                       # @param [Boolean] mutable whether the cached file should be mutable
                       def initialize(cache_root, uri, mutable)
-                        key            = URI.escape(uri.sanitize_uri, ':/')
+                        key            = URI.escape(uri.sanitize_uri, ':/&')

Contributor

nebhale Oct 10, 2016

Not many people have the courage to submit a change to the core functionality 😉 Definitely a needed change though.

Contributor Author

aloismayr Oct 10, 2016

I'm sorry I don't wanted to smuggle this into the core functionality with the PR. Hope this was okay tough. Next time I'll talk to you first - or create an issue.

Contributor

nebhale Oct 10, 2016

No, no! This was a perfect change that I'm happy to have. Just most people never dive deeply enough into the code to know what they need to do. More like this please.


          Enhanced Dynatrace integrations:

805ef8b

 - Renamed Ruxit to Dynatrace OneAgent
 - Renamed DynaTrace to Dynatrace Appmon
 - Updated detection criterion for Dynatrace OneAgent (SaaS/Managed)
	- tenant, tenanttoken and server are deprecated (but still supported for the sake of backward compatibilty)
	- environmentid, apitoken and endpoint are the new required fields
 - Dynatrace OneAgent will be downloaded from Dynatrace SaaS/Managed cluster via REST API (if apitoken given) rather than from the public repository
 - Public repository will still be maintained for ensuring backward compatibility
 - Updated documentation and tests
 - Minor bugfix to support caching of HTTP downloads with query parameters

nebhale merged commit 805ef8b into cloudfoundry:master

aloismayr mentioned this pull request

Enhance and improve Dynatrace integrations cloudfoundry/ibm-websphere-liberty-buildpack#317

Merged

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

Labels

None yet