Multiplexing fix

[ericmustin]

Updated:

This PR is a first step at addressing #1204 .

As far as I can tell our multiplexing code that relied on describes blocks for http clients was never really functional. It appears that, due to the way our PatternResolver attempts to resolve a key, we woulld always "fallback" to :default configuration if that describes pattern has not yet been added, which leads to methods like configuration_for?(key) to always return true. And since configuration_for?(key) always returns true when using a PatternResolver, we never add the describes pattern to the configurations, instead always just overwriting the default, seen here:

config = configuration_for?(key) ? configuration(key) : add_configuration(key)

I'm a bit hesitant to modify too much here, as it's unclear to me whether the root issue here is that The root issue here is that

A: our PatternResolver is returning an unexpected :default as a fallback incorrectly.

B: Or, our configuration_for?(key) method is incorrectly using resolver.resolve to determine whether configuration explicitly exists.

So in the meantime I've put in a patch, i'd like to see how this runs against the whole test suite as well. Open to feedback as well, just wanted to start the conversation here.

B: Additionally, the test suite for integations that relied on PatternResolver were testing with a describe block, not a describes block. an incorrectly spelled key effectively functions as overwriting the defaults.

C: And, lastly, even with the above issues fixed, the PatternResolver resolve method was not properly accounting for Regex patterns, which is the only pattern we document as being accepted by describes

So, needless to say there were a few things at work here. Here's what i've done in this PR to clean this up going forward.

1. PatternResolver resolve method now explicitly returns nil in the event the pattern has not yet been added. While a resolver's resolve method could hypothetically have a fallback value, given the way it's being used in practice within our configuration code, having it return a fallback value is problematic.

2. the resolve method also now has logic to correctly resolve against Regexp patterns.

3. Tests are updated to test against the appropriate describes block, and i've added "non-matching" describes blocks to prevent a regression from what is happening in Unable to configure several http services with different service names #1204 where the last describes block to be configured "wins".

4. The Excon integration had some unusual behavior for how it was determining precedence for it's default configuration code vs it's middleware configuration code vs it's describes configuration code. I've fixed this to support both describes taking precedence over default and middleware taking precedence over default.

I think technically this constitutes a breaking change since it's possible some users have been relying on describes code that otherwise wouldn't have "matched" to set config details. So, for example, even if the user put describes: localHoost, {service_name: xyz} instead of describes: localhost, {service_name: xyz}, the service_name would still be set to xyz while this would now be fixed, we should make sure to not this in release notes.

Lmk what you think

[ericmustin]

Something is still broken here, a lot of the analytics and sampling specs are failing

[marcotc]

Nice catch!

[ericmustin]

I've updated this PR description to match current work

[marcotc]

I recommend using #=== for all the supported types, like we do in our SimpleMatcher. In this case, I don't think we should type check, but instead document that patterns must respond to #===.

You might need to #to_s the name, but I'm not sure on that.

[ericmustin]

So i've updated this whole pattern to the, far simpler

          def resolve(name)
            # Try to find a matching pattern
            matching_pattern = patterns.find do |pattern|
              if pattern.respond_to?(:===)
                (pattern === name) || (pattern === name.to_s) || (pattern == name)
              end
            end

            # Return match or default
            matching_pattern
          end

For the cases in (pattern === name) || (pattern === name.to_s) || (pattern == name):

• The first case is needed for when the pattern is a Proc or Regexp and the name is a string (or whatever the Proc or Regexp expect as input). As far as I can tell the Proc === implementation is an alias for .call and Regexp === implementation is equivalent to regex =~ str >= 0
• The second case is needed for when the pattern is a Regexp but the name is something that coerces to a string, like URL('<url>'), which is part of our pattern resolver spec.
• The final case is needed when the pattern is a regexp or proc and then name is also a Regexp or Proc, which is how resolver.resolve gets used within the Configurable add_configuration method

Lmk what you think

[marcotc]

Can't case 1 and 2 be handled only by 2?

Also, I don't see how in 3 we'd receive a proc or regex as the name parameter. I tried to follow the method you linked, but it seems like we are receiving a regular key there. Also, was this case supported before? I don't see a raw == in the original code.

[ericmustin]

1 and 2 are in opposition unfortunately if we aren't going to differentiate between a Proc and Regexp

Option 1 covers the case where we don't want to coerce the name for procs, this is covered in spec here for Procs

Option 2 covers the case where we do want to coerce the name for regexp, this is covered in spec here for Regexp

That being said I can update this to check explicitly for a Proc to decide between using option 1 or 2. Also can remove the explicit respond_to?(:===).

The need for option 3 is because we use resolver.resolve with key inputs that are both

• the actal host, url string or string coercablle that we want to match against a pattern, like here

• but also the :describes block (potentially a proc or regex) itself, seen here.

There's a lot of room for improvement in the way we're using resolvers but for now to fit the extisting usage the pattern resolver has to be pretty permissive with what it can resolve against