Update env var conventions and make consistent across language clients #1115

ericmustin · 2020-07-23T14:24:41Z

This PR updates the conventions of currently existing key environment variables used for configuration the tracer and it's integrations, as well as adds a few new environment variables for key configurability that are supported in some other language clients. The goal of this PR is to standardise the env var's available for configuring the tracer while still supporting older environment variables

New Env Vars

DD_TRACE_AGENT_URL : The URL of the Trace Agent that the tracer submits to. Takes priority over hostname (DD_AGENT_HOST) and port (DD_TRACE_AGENT_PORT), if set. currently only supports http/https.
DD_TRACE_DEBUG: Enables or disables debug logging.
DD_TRACE_<INTEGRATION>_ENABLED: Allows user to ensure instrumentation for a specific integration (eg: DD_TRACE_RAILS_ENABLED) is disabled. the standard across languages is _ENABLED but since it isn't possible to enable solely via env var at the moment, and must be done in code, this could be confusing to users. ~~So adding _DISABLED so users still have the functionality to disable instrumentation for testing or performance purposes.~~ Adding documentation on this restriction.
DD_TRACE_ENABLED: Allows user to disable the tracer. The spans are still created but will not be flushed to the agent

Updated Env Vars

Migration DD_<INTEGRATION>_ANALYTICS_ENABLED to DD_TRACE_<INTEGRATION>_ANALYTICS_ENABLED: Updates naming convention for enabling/disabled analyzed spans for a specific integration, such as DD_TRACE_RAILS_ANALYTICS_ENABLED. Deprecated DD_<INTEGRATION>_ANALYTICS_ENABLED is still supported.
Migration DD_<INTEGRATION>_ANALYTICS_SAMPLE_RATE to DD_TRACE_<INTEGRATION>_ANALYTICS_SAMPLE_RATE: Updates naming convention for enabling/disabled analyzed spans for a specific integration, such as DD_TRACE_RAILS_ANALYTICS_SAMPLE_RATE. Deprecated DD_<INTEGRATION>_ANALYTICS_SAMPLE_RATE is still supported.

Notes

should we scrap the DD_TRACE_INTEGRATION_DISABLED since it's off spec or move toDD_TRACE_INTEGRATION_ENABLED to stay on spec(but document that it only can be used to disable instrumentation)? i think it could provide value for users either way. updated to DD_TRACE_INTEGRATION_ENABLED with a note that only false is a valid option.
Not all of these env vars are clearly documented in docs, or don't have a clear place to add documentation., such as DD_TRACE_<INTEGRATION>_ANALYTICS_ENABLED, DD_TRACE_AGENT_URL, DD_TRACE_<INTEGRATION>_DISABLED. Not sure why a lot of these existing env vars haven't been documented but if we need to document them now we could add a seperate 'misc env vars` section to the docs maybe? Added to docs
In general, would be good to have feedback on the way the new env vars have been implemented. I tried to fit them into the existing conventions we have.

…racer, trace agent url

…ed env var

…spec and formattinng

brettlangdon · 2020-07-23T14:34:11Z

lib/ddtrace/contrib/action_pack/ext.rb

@@ -5,6 +5,7 @@ module ActionPack
      module Ext
        APP = 'action_pack'.freeze
        ENV_ANALYTICS_ENABLED = 'DD_ACTION_PACK_ANALYTICS_ENABLED'.freeze


Suggested change

ENV_ANALYTICS_ENABLED = 'DD_ACTION_PACK_ANALYTICS_ENABLED'.freeze

ENV_ANALYTICS_ENABLED = 'DD_TRACE_ACTION_PACK_ANALYTICS_ENABLED'.freeze

👍 updated

brettlangdon · 2020-07-23T14:34:59Z

lib/ddtrace/contrib/action_cable/ext.rb

@@ -4,7 +4,8 @@ module ActionCable
      # ActionCable integration constants
      module Ext
        APP = 'action_cable'.freeze
-        ENV_ANALYTICS_ENABLED = 'DD_ACTION_CABLE_ANALYTICS_ENABLED'.freeze
+        ENV_ANALYTICS_ENABLED = 'DD_TRACE_ACTION_CABLE_ANALYTICS_ENABLED'.freeze
+        ENV_ANALYTICS_ENABLED_OLD = 'DD_ACTION_CABLE_ANALYTICS_ENABLED'.freeze
        ENV_ANALYTICS_SAMPLE_RATE = 'DD_ACTION_CABLE_ANALYTICS_SAMPLE_RATE'.freeze


These need to change as well, DD_TRACE_<INTEGRATION>_ANALYTICS_SAMPLE_RATE, sorry if the spec wasn't clear about that

gotcha, updated

brettlangdon · 2020-07-23T14:36:06Z

lib/ddtrace/contrib/extensions.rb

+          def integration_disabled?(name)
+            env_to_bool(
+              "#{ENV_INTEGRATION_DISABLED_PREFIX}_"\
+              "#{name.to_s.sub(':', '').upcase}"\


will this always match the same name used in DD_TRACE_<NAME>_ANALYTICS_ENABLED that you set above?

yup, it's the key being passed into c.use which are all either one word or snake case. I think this is just a convention though and not enforced anywhere afaik, so should probably add a test to prevent a regression at some point.

brettlangdon · 2020-07-23T14:38:42Z

lib/ddtrace/environment.rb

@@ -6,7 +6,12 @@ module Environment
    # Defines helper methods for environment
    module Helpers
      def env_to_bool(var, default = nil)
-        ENV.key?(var) ? ENV[var].to_s.strip.downcase == 'true' : default
+        if var.is_a?(Array)


could do:

var = [var] unless var.is_a?(Array) env_match = var.find { |env_var| ENV.key?(env_var) } env_match ? ENV[env_match].to_s.strip.downcase == 'true' : default

or:

if var.is_a?(Array) env_match = var.find { |env_var| ENV.key?(env_var) } else env_match = ENV.key?(var) end env_match ? ENV[env_match].to_s.strip.downcase == 'true' : default

Either could simplify/DRY this a little bit

(just a suggestion, not a requirement)

updated to be a bit dry'er

brettlangdon · 2020-07-23T14:41:05Z

lib/ddtrace/ext/diagnostics.rb

@@ -2,7 +2,8 @@ module Datadog
  module Ext
    module Diagnostics
      DD_TRACE_STARTUP_LOGS = 'DD_TRACE_STARTUP_LOGS'.freeze
-
+      DD_TRACE_DEBUG_LOGS = 'DD_TRACE_DEBUG'.freeze


s/_LOGS//?

makes sense, updated

brettlangdon · 2020-07-23T14:43:33Z

lib/ddtrace/transport/http.rb

        ENV.fetch(Datadog::Ext::Transport::HTTP::ENV_DEFAULT_PORT, Datadog::Ext::Transport::HTTP::DEFAULT_PORT).to_i
      end

+      def default_url


what we've done in other languages is use DD_AGENT_HOST and DD_TRACE_AGENT_PORT to build the url, that way once the app loads/is configured, then we only have a url we are working with.

https://github.com/DataDog/dd-trace-py/blob/2dff53f643f71fc4b9f414a5d85961ab130d99a1/ddtrace/tracer.py#L74-L79

Not sure exactly how that translates here, since I am guessing default_hostname/default_port are what are used to create the connection?

I think a bit lower down hostname and port get coerced into a url, but it's a bit tricky since we're resolving the priority of host / port in code > url env var > host/port env vars here, before the url gets formed. which is why there's this roundabout "extract the hostname/port from the url being set via env var", to avoid having to re-structure the other parts of transport too much. @marcotc may have a cleaner solution/idea here.

I think this is ok for now, but I agree that supporting only url input is a better way forward.
It requires more changes to the transport that it would be comfortable for this PR, though.

brettlangdon · 2020-07-23T14:44:06Z

lib/ddtrace/transport/http.rb

+        if url_env
+          uri_parsed = URI.parse(url_env)
+
+          uri_parsed if %w[http https].include?(uri_parsed.scheme)


is https honored anywhere? if I set https://127.0.0.1:8106/ will that actually use TLS?

I ...don't think so? mainly I just wanted to avoid a malformed url being passed in

…ys in env conversion helpers

…w env vars, use simpler env var names

brettlangdon · 2020-07-23T22:21:47Z

lib/ddtrace/environment.rb

-        else
-          ENV.key?(var) ? ENV[var].to_s.strip.downcase == 'true' : default
-        end
+        var = var.find { |env_var| ENV.key?(env_var) } if var.is_a?(Array)


marcotc

Looks pretty good overall, great stuff!
I left a few suggestions in the review.

docs/GettingStarted.md

marcotc · 2020-07-24T17:56:20Z

lib/ddtrace/contrib/extensions.rb

@@ -56,7 +58,7 @@ def [](integration_name, configuration_name = :default)
          def instrument(integration_name, options = {}, &block)
            integration = fetch_integration(integration_name)

-            unless integration.nil?
+            unless integration.nil? || !integration_enabled?(integration_name)


I think this can be more cleanly implemented by checking !integration.default_configuration.enabled.
All integrations already have a default_configuration method, but they don't all have enabled defined.

You would define enabled in this shared class: https://github.com/DataDog/dd-trace-rb/blob/master/lib/ddtrace/contrib/configuration/settings.rb
An integration-specific overwrite would still need to be created to support environment variable configuration, like we do for analytics_enabled today.

makes sense, i went ahead and took this approach and defined enabled in the shared class as well as each Contrib's setting's, and added the relevant Env var constants the respective ext files.

One thing is that it required me to modify some of the fixtures in the extensions_spec, and that i added a very small test in the configurable_spec to test the shared class. I found this pretty difficult to easily test at the individual contrib level, and i've punted on it in this PR for now. I think it will take some work done ala the analytics_examples / analytics_spec that abstracts everything out into shared helpers but the scope of that work could take a bit and didn't want it to be a blocker for this PR.

lib/ddtrace/environment.rb

marcotc · 2020-07-24T18:29:43Z

lib/ddtrace/transport/http.rb

        ENV.fetch(Datadog::Ext::Transport::HTTP::ENV_DEFAULT_PORT, Datadog::Ext::Transport::HTTP::DEFAULT_PORT).to_i
      end

+      def default_url


I think this is ok for now, but I agree that supporting only url input is a better way forward.
It requires more changes to the transport that it would be comfortable for this PR, though.

…rray support to all env_to_x helpers

…tting level

ericmustin · 2020-07-27T08:43:01Z

@marcotc tried to address all the feedback here, i've re-requested a review when you have time. had one note on testing the env_enabled integration environment variable that I could your opinion on.

Requested changes were addressed

marcotc

🔥

ericmustin added 7 commits July 22, 2020 15:55

Added: env var for enabling debug logs

537cafe

[feat]: WIP add env var standards around debug logging, disablilng tr…

0a990c2

…racer, trace agent url

[feat]: remove typo from writer

6cc1c43

[feat]: rename analytics enabled env var and add support for deprecat…

ef097d7

…ed env var

[feat]: WIP enable/disable for integration via env var

bc6d537

[feat]: add ability to disable specific integrations via env var add …

f17379f

…spec and formattinng

[docs] update readme for new env vars

f811873

ericmustin requested a review from a team July 23, 2020 14:24

[tests]: move variable call to inside test

d10e829

brettlangdon reviewed Jul 23, 2020

View reviewed changes

ericmustin added 4 commits July 23, 2020 23:13

[feat]: sample rate env var rename and support old format, allow arra…

6c5e1ed

…ys in env conversion helpers

[feat]: support setting multiple sample rate env vars and document ne…

b75be5d

…w env vars, use simpler env var names

[fix]: env var key lookup check for nil

3a8bcb0

[fix]: update env var for contrib

3d2f8e6

brettlangdon reviewed Jul 23, 2020

View reviewed changes

brettlangdon requested a review from marcotc July 23, 2020 22:22

marcotc previously requested changes Jul 24, 2020

View reviewed changes

ericmustin added 4 commits July 25, 2020 13:47

[docs]: update env var documentation to include defaults and extend a…

14c8bbe

…rray support to all env_to_x helpers

[feat]: add env var to disable integrations at integration contrib se…

7e6b69e

…tting level

[fix]: rubocop for specs

792bd1a

[tests]: add test for enabled key on default config

db0442b

ericmustin requested a review from marcotc July 27, 2020 08:41

marcotc approved these changes Jul 27, 2020

View reviewed changes

ericmustin merged commit 8d54207 into master Jul 27, 2020

This was referenced Jul 28, 2020

Disable DD Ruby tracer in the same way as DD Java tracer #941

Closed

feat: Integrate Sneakers into ddtracer #1121

Merged

marcotc assigned ericmustin Jul 28, 2020

marcotc added this to the 0.39.0 milestone Jul 28, 2020

marcotc mentioned this pull request Aug 4, 2020

gem doesn't honor DISABLE_DATADOG_AGENT env var #1130

Closed

marcotc linked an issue Aug 4, 2020 that may be closed by this pull request

gem doesn't honor DISABLE_DATADOG_AGENT env var #1130

Closed

ivoanjo deleted the update_env_var_conventions branch July 16, 2021 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update env var conventions and make consistent across language clients #1115

Update env var conventions and make consistent across language clients #1115

ericmustin commented Jul 23, 2020 •

edited

Loading

brettlangdon Jul 23, 2020

ericmustin Jul 23, 2020

brettlangdon Jul 23, 2020

ericmustin Jul 23, 2020

brettlangdon Jul 23, 2020

ericmustin Jul 23, 2020

brettlangdon Jul 23, 2020

brettlangdon Jul 23, 2020

ericmustin Jul 23, 2020

brettlangdon Jul 23, 2020

ericmustin Jul 23, 2020

brettlangdon Jul 23, 2020

ericmustin Jul 23, 2020

marcotc Jul 24, 2020

brettlangdon Jul 23, 2020

ericmustin Jul 23, 2020

brettlangdon Jul 23, 2020

marcotc left a comment

marcotc Jul 24, 2020

ericmustin Jul 27, 2020

marcotc Jul 24, 2020

ericmustin commented Jul 27, 2020

marcotc left a comment

	ENV_ANALYTICS_ENABLED = 'DD_ACTION_PACK_ANALYTICS_ENABLED'.freeze
	ENV_ANALYTICS_ENABLED = 'DD_TRACE_ACTION_PACK_ANALYTICS_ENABLED'.freeze

Update env var conventions and make consistent across language clients #1115

Update env var conventions and make consistent across language clients #1115

Conversation

ericmustin commented Jul 23, 2020 • edited Loading

New Env Vars

Updated Env Vars

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmustin commented Jul 27, 2020

marcotc left a comment

Choose a reason for hiding this comment

ericmustin commented Jul 23, 2020 •

edited

Loading