Add ability to specify service names by domain #882

stormsilver · 2019-12-10T22:50:45Z

What?

When using the faraday and excon contribs with the split_by_domain feature turned on, allow users to specify an additional hash that can override the service name by domain.

Why?

We use an external service that makes a per-customer domain name. When we create HTTP requests to this service for our customers, it looks like user-123.foo.com and user-abc.foo.com (and so on). When we split by domain (which we want to do for all the other HTTP services we use), it ends up creating hundreds of separate "services" on the service map - all of which are really the same external service. We'd like to have these all show up as one service.

How?

By providing a hash in the form of {/regex/ => 'desired_service_name'}, we can match the desired domain names (/user-[^.]+\.foo\.com/) and provide the service name we want to see instead (user.foo.com)

delner · 2019-12-11T23:50:17Z

Thanks for the PR @stormsilver! I've been wanting to do something like this for a while, so I'm really excited to see this.

Couple of quick thoughts:

We probably would like to apply this behavior more generally to other HTTP integrations, too. Maybe in this PR if it ends up not being much work, but maybe in another PR if it is. It'd be cool if the mapping/configuration option were generic enough that we could apply it to other integrations easily.
We have a configuration multiplexing feature we might want to leverage to do this. The idea is you'd do something like Datadog.configure[:faraday, /regex/][:service_name] = 'custom-service-name' rather than having to define and provide a map option. Basically the integration defines a Resolver, then it's used to map values to keys to select the correct configuration settings. In this case it would use the regex matcher. We currently do this in ActiveRecord and in Dalli; see the code that would enable this.

I think is definitely something we'd like to merge though those things considered.

delner · 2019-12-12T01:29:09Z

So I took a crack at implementing this via resolver pattern instead, and came up with this. You would use it like:

Datadog.configure do |c|
  c.use :faraday, describe: /example\.com/ do |faraday|
    faraday.service_name = 'bar'
    faraday.split_by_domain = false # Only necessary if split_by_domain is true by default
  end
end

Tests pass so it seems to work. If we would want to incorporate this into the PR, some other things we'd have to do is:

Consider changing how the RegexpResolver adds keys to itself. Right now if you call it ever with an Regexp, it will cache that Regexp (e.g. Datadog.configuration[:faraday, /some exp/]). There might be a better way to be explicit about adding vs accessing that's safer.
Add unit tests for it.
Implement it for the other HTTP libraries.
Update the documentation.

I didn't have permissions to push to your forked repo, so I'm sharing the diff here:

regexp_resolver.txt

Feedback welcome!

stormsilver · 2019-12-12T23:04:28Z

I applied you patch locally and spent some time spelunking the code to see how the resolvers work. This makes sense to me. Implementing this on all 4 of the HTTP libraries will require some solid refactoring of ethon and http. It makes the most sense to me to begin extracting stuff out into an HTTP tracer helper module. I will work on this in my spare time. It’ll be a bit before it’s done.

…

-- Eric

On Dec 11, 2019, at 19:29, David Elner ***@***.***> wrote: So I took a crack at implementing this via resolver pattern instead, and came up with this. You would use it like: Datadog.configure do |c| c.use :faraday, describe: /example\.com/ do |faraday| faraday.service_name = 'bar' faraday.split_by_domain = false # Only necessary if split_by_domain is true by default end end Tests pass so it seems to work. If we would want to incorporate this into the PR, some other things we'd have to do is: Consider changing how the RegexpResolver has keys to it. Right now if you call it ever with an Regexp, it will cache that Regexp (e.g. Datadog.configuration[:faraday, /some exp/]). There might be a better way to be explicit about adding vs accessing that's safer. Add unit tests for it. Implement it for the other HTTP libraries. Update the documentation. I didn't have permissions to push to your forked repo, so I'm sharing the diff here: regexp_resolver.txt Feedback welcome! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

delner · 2019-12-13T02:30:13Z

No worries, let me know if there's anything I can be of help with, particularly as it pertains to the resolver/configuration pattern.

stormsilver

Okay, this has been updated with your comments. Please let me know if I forgot something. In particular I dunno if the specs are up to snuff.

stormsilver · 2019-12-13T21:18:04Z

lib/ddtrace/contrib/configurable.rb

-        def configure(key, options = {}, &block)
-          key = resolver.resolve(key || :default)
+        def configure(key = :default, options = {}, &block)
+          resolver.add_key(key)


This is how I ended up solving the awkward dance that RegexpResolver was doing to remember patterns.

stormsilver · 2019-12-13T21:21:24Z

lib/ddtrace/contrib/ethon/easy_patch.rb

@@ -23,14 +27,13 @@ def http_request(url, action_name, options = {})
          end

          def headers=(headers)
-            return super unless tracer_enabled?


I had to remove the tracer_enabled? call here and in reset because, since we added the resolver pattern, you now have to have a hostname in order to determine the configuration (to decide if the tracer is enabled. theoretically you can now disable the tracer for requests matching /do-not-want.io/)... but you can set headers before supplying a URL to net/http.

This will have the side effect of creating a few DD instance variables on the net/http object even if the tracer is disabled.

Not sure I understand entirely; would it make sense to derive enabled? from the default config if the hostname isn't available? It might mean that not all settings are strictly respected per host, but maybe that's okay? Thoughts are most welcome.

My thought was that it's better to respect all settings per host and leave a few extra ivars on the request object rather than have a surprising situation where you disabled the tracer for certain hosts but they are still getting traced (which would appear to the casual observer to be a bug).

I can certainly derive enabled? from the default config if there's no hostname if you feel like the tradeoff isn't worth it.

stormsilver · 2019-12-13T21:25:40Z

lib/ddtrace/contrib/http/instrumentation.rb

-              service = Datadog.configuration[:http][:service_name]
-              tracer = Datadog.configuration[:http][:tracer]
+              service = config[:service_name]
+              tracer = config[:tracer]


There's a subtle bug here in that someone could call datadog_pin on this object with no args before running a request. It would then use the :default configuration to supply args to the pin. If the request is then run for a hostname which matches a regex configuration, we'll use the wrong configuration since it's been memoized.

I didn't have enough context to know if this is actually a problem, or what people use custom service names on pins for, or if the pin should just be removed since they look deprecated, or ...

Yeah I think this could be a problem, given that someone might build an HTTP object, set something on its datadog_pin then call it with a request resulting in this scenario.

I think that's one of the tricky things about this "by hostname" configuration: we have 1) default, 2) "by host" and 3) by object configuration which all has to be merged together to apply the settings that make the most sense for usability. Inevitably I think this resolution has to happen at request time (less than ideal, but unavoidable.)

stormsilver · 2019-12-13T22:54:51Z

Well, the CI tests failed which led me to realize that I had put the ethon tests in the wrong spot and they weren't even running except on CI (I thought I had run them) which led me to realize that I hadn't even implemented the service_name change for ethon, which is really embarrassing. So these failures are quite legit.

delner · 2019-12-14T22:22:34Z

Haha, that's quite already @stormsilver. I'll try to take a look at your changes see if I can give any helpful feedback.

delner

So far looking pretty good. Just left a few comments to consider, but I'm happy with the direction this is heading in.

delner · 2019-12-14T22:24:46Z

lib/ddtrace/contrib/configuration/resolver.rb

@@ -3,6 +3,10 @@ module Contrib
    module Configuration
      # Resolves a value to a configuration key
      class Resolver
+        def add_key(key)


I think adding a second method here was inevitable, so I'm on board with this.

lib/ddtrace/contrib/configurable.rb

delner · 2019-12-14T22:29:48Z

lib/ddtrace/contrib/configuration/resolvers/regexp_resolver.rb

+        # Matches strings against Regexps.
+        class RegexpResolver < Datadog::Contrib::Configuration::Resolver
+          def add_key(pattern)
+            patterns << pattern.is_a?(Regexp) ? pattern : /#{pattern}/


Does pattern in /#{pattern}/ need to be made regex safe? I thought there was some Regexp parsing you could apply, but maybe that's only relevant to literals.

Ah, yes. Regexp.quote is what you're talking about, I think: https://ruby-doc.org/core-2.7.0/Regexp.html#method-c-quote

delner · 2019-12-14T22:55:27Z

lib/ddtrace/contrib/ethon/easy_patch.rb

@@ -23,14 +27,13 @@ def http_request(url, action_name, options = {})
          end

          def headers=(headers)
-            return super unless tracer_enabled?


Not sure I understand entirely; would it make sense to derive enabled? from the default config if the hostname isn't available? It might mean that not all settings are strictly respected per host, but maybe that's okay? Thoughts are most welcome.

delner · 2019-12-14T22:58:43Z

lib/ddtrace/contrib/excon/integration.rb

@@ -26,6 +27,10 @@ def default_configuration
        def patcher
          Patcher
        end
+
+        def resolver


Not important right now, but maybe we could extract this behavior to some kind of HTTP module that can be recomposed into each integration that uses it. I don't think we have to do that right now though, as we probably should wait to get more behavior worth extracting first.

delner · 2019-12-14T23:06:15Z

lib/ddtrace/contrib/http/instrumentation.rb

-              service = Datadog.configuration[:http][:service_name]
-              tracer = Datadog.configuration[:http][:tracer]
+              service = config[:service_name]
+              tracer = config[:tracer]


Yeah I think this could be a problem, given that someone might build an HTTP object, set something on its datadog_pin then call it with a request resulting in this scenario.

I think that's one of the tricky things about this "by hostname" configuration: we have 1) default, 2) "by host" and 3) by object configuration which all has to be merged together to apply the settings that make the most sense for usability. Inevitably I think this resolution has to happen at request time (less than ideal, but unavoidable.)

delner · 2019-12-14T23:11:25Z

lib/ddtrace/contrib/http/instrumentation.rb

-            pin = datadog_pin
+            host, = host_and_port(req)
+            request_options = datadog_configuration(host)
+            pin = datadog_pin(request_options)
            return super(req, body, &block) unless pin && pin.tracer

            if Datadog::Contrib::HTTP.should_skip_tracing?(req, @address, @port, pin.tracer)


Something that's really cool about this resolver is we might be able to use it to remove this circuit breaker (which exists to prevent our tracer from tracing itself when it writes traces over HTTP to the agent.) Alternatively, we could probably use enabled as well to effect the same result.

Probably should be addressed in another PR though.

This allows all integrations that trace HTTP requests to: - use the domain name of the request as the service name - specify a service name for requests matching a regex

stormsilver · 2020-01-02T15:19:45Z

Okay @delner I think this is ready for another review.

delner · 2020-01-02T17:42:56Z

Awesome, I'll give it another look.

delner

Shaping up really nicely now @stormsilver! I think we're really close now. Just a few small suggestions for some tweaks.

Documentations changes are good; are there any other tests we should add to cover the new behavior?

delner · 2020-01-02T19:44:07Z

lib/ddtrace/contrib/configuration/resolvers/regexp_resolver.rb

+        # Matches strings against Regexps.
+        class RegexpResolver < Datadog::Contrib::Configuration::Resolver
+          def add_key(pattern)
+            patterns << pattern.is_a?(Regexp) ? pattern : /#{Regexp.quote(pattern)}/


Although its kind of nice to convert everything to a Regex for consistency I think there's a couple of problems with interpolating strings to Regexp:

String values might match inadvertently, e.g. passing 'a' would match 'a' and 'aa'.

There's an added expense to evaluating Regexp when doing == with a string might suffice.

I would recommend accepting any object, then in #resolve do a case statement such that if the pattern is a Regexp, do =~, otherwise coerce the pattern to a string and do ==.

delner · 2020-01-02T19:45:40Z

lib/ddtrace/contrib/configuration/resolvers/regexp_resolver.rb

+            return :default if name == :default
+
+            name = name.to_s
+            matching_pattern = patterns.find { |p| p =~ name }


Maybe this isn't a big deal, but should we change name to name.to_s? Coercion might prevent some type incompatibility issues such as with URI, etc.

delner · 2020-01-02T19:51:30Z

lib/ddtrace/contrib/http_annotation_helper.rb

+module Datadog
+  module Contrib
+    # Contains methods helpful for tracing/annotating HTTP request libraries
+    module HttpAnnotationHelper


Should we extract this?

Although it does reduce some repetition, it might be a bit premature given its this single line function. I might favor the repetition over having the additional constant in this case; the use of options specific to the integration (such as service_name and split_by_domain I think reinforces this right now, too. Eventually if the behavior grows in scope, an extraction like this might be appropriate.

delner · 2020-02-05T22:39:57Z

To support a similar "multiplexing" scenario where we would benefit from the resolver changes, I've extracted those changes to #940 and merged them to master. We can rebase this PR on those changes which should reconcile things a bit.

delner · 2020-02-19T22:37:53Z

Hey @stormsilver , looks like this PR has grown stale, but we still wanted to see these changes incorporated. Given your repo doesn't grant push permissions to upstream repo members, I opened #953 which replaces this PR to rebase your commits and apply a few changes of my own.

We'll continue working on this feature in that PR; please feel free to review/contribute to that as you wish. Thanks again for your contributions so far!

stormsilver requested a review from a team December 10, 2019 22:50

stormsilver force-pushed the domain_map branch from ccf8fa5 to deb8a69 Compare December 11, 2019 17:17

stormsilver commented Dec 13, 2019

View reviewed changes

stormsilver force-pushed the domain_map branch 2 times, most recently from 8723354 to e389f23 Compare December 13, 2019 21:50

delner reviewed Dec 14, 2019

View reviewed changes

stormsilver force-pushed the domain_map branch 2 times, most recently from 514d14c to 997d651 Compare December 27, 2019 22:38

stormsilver added 3 commits December 27, 2019 16:52

Add ability to specify service names by domain

0cf6abd

Implement regex resolver on all HTTP integrations

1cb66f4

This allows all integrations that trace HTTP requests to: - use the domain name of the request as the service name - specify a service name for requests matching a regex

Fixup

4cc04a0

stormsilver force-pushed the domain_map branch from 997d651 to 4cc04a0 Compare December 27, 2019 22:52

delner reviewed Jan 2, 2020

View reviewed changes

This was referenced Jan 27, 2020

APM redis reporting by host #861

Closed

Feature: Redis multiplexing #937

Merged

delner mentioned this pull request Feb 4, 2020

Add #add to configuration resolver #940

Merged

delner mentioned this pull request Feb 19, 2020

Add HTTP multiplexing #953

Merged

delner closed this Feb 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to specify service names by domain #882

Add ability to specify service names by domain #882

stormsilver commented Dec 10, 2019

delner commented Dec 11, 2019 •

edited

Loading

delner commented Dec 12, 2019 •

edited

Loading

stormsilver commented Dec 12, 2019 via email

delner commented Dec 13, 2019

stormsilver left a comment

stormsilver Dec 13, 2019

stormsilver Dec 13, 2019

delner Dec 14, 2019

stormsilver Dec 27, 2019

stormsilver Dec 13, 2019

delner Dec 14, 2019

stormsilver commented Dec 13, 2019

delner commented Dec 14, 2019

delner left a comment

delner Dec 14, 2019

delner Dec 14, 2019

stormsilver Dec 27, 2019

delner Dec 14, 2019

delner Dec 14, 2019

delner Dec 14, 2019

delner Dec 14, 2019

stormsilver commented Jan 2, 2020

delner commented Jan 2, 2020

delner left a comment

delner Jan 2, 2020

delner Jan 2, 2020

delner Jan 2, 2020

delner commented Feb 5, 2020

delner commented Feb 19, 2020

Add ability to specify service names by domain #882

Add ability to specify service names by domain #882

Conversation

stormsilver commented Dec 10, 2019

What?

Why?

How?

delner commented Dec 11, 2019 • edited Loading

delner commented Dec 12, 2019 • edited Loading

stormsilver commented Dec 12, 2019 via email

delner commented Dec 13, 2019

stormsilver 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stormsilver commented Dec 13, 2019

delner commented Dec 14, 2019

delner 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

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

stormsilver commented Jan 2, 2020

delner commented Jan 2, 2020

delner 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

delner commented Feb 5, 2020

delner commented Feb 19, 2020

delner commented Dec 11, 2019 •

edited

Loading

delner commented Dec 12, 2019 •

edited

Loading