browser monitoring injection logic hardening

[fallwith]

These changes introduce hardening to the logic that determines whether or not to inject the browser agent script tag.

In particular, the following 2 issues have been addressed:

1. If either #traced_call or #should_instrument? encounter any exceptions, these exceptions should be handled and logged without impacting the observed web application.
2. Allow a response headers hash to use symbols for values.

Closes #2462

[fallwith]

NOTE to @kaylareopelle and @hannahramadan: I was worried about introducing performance overhead while supporting hash values as symbols. After some benchmarking, the solution I came up with is to use #match? which is defined on both Symbol and String. The switch to using #match? should actually introduce a small performance boost to all existing users, while also adding support for hash-values-as-symbols users.

Here is the benchmarking code in question:

require 'benchmark'

SYMBOLS = { :'an/example' => :'a/value',
            :'another-example' => :'another-value',
            :content_type => :'text/html',
            :traceparent => :'00-da8bc8cc6d062849b0efcf3c169afb5a-7d3efb1b173fecfa-01',
            :tracestate => :'33@nr=0-0-33-2827902-7d3efb1b173fecfa-e8b91a159289ff74-1-1.234567-1518469636035' }

STRINGS = SYMBOLS.each_with_object({}) { |(k,v), h| h[k.to_s] = v.to_s }
TEXT_HTML = 'text/html'
TEXT_HTML_PATTERN = %r{text/html}.freeze
TIMES = 1_000_000

puts 'Finding a speedy way to test symbol text inclusion...'

Benchmark.bm do |x|
  x.report 'baseline' do
    TIMES.times do
      # use STRINGS twice, as SYMBOLS will error out
      STRINGS.each { |k, v| STRINGS[k].include?(TEXT_HTML) }
      STRINGS.each { |k, v| STRINGS[k].include?(TEXT_HTML) }
    end
  end

  x.report '#to_s' do
    TIMES.times do
      SYMBOLS.each { |k, v| SYMBOLS[k].to_s.include?(TEXT_HTML) }
      STRINGS.each { |k, v| STRINGS[k].to_s.include?(TEXT_HTML) }
    end
  end

  x.report '#match? with pattern' do
    TIMES.times do
      SYMBOLS.each { |k, v| SYMBOLS[k].match?(TEXT_HTML_PATTERN) }
      STRINGS.each { |k, v| STRINGS[k].match?(TEXT_HTML_PATTERN) }
    end
  end

  x.report '#match? with string' do
    TIMES.times do
      SYMBOLS.each { |k, v| SYMBOLS[k].match?(TEXT_HTML) }
      STRINGS.each { |k, v| STRINGS[k].match?(TEXT_HTML) }
    end
  end
end

[kaylareopelle]

I really like these updates, @fallwith. Especially with the performance boost!

I think we might want a changelog entry for this. What do you think?

[fallwith]

I really like these updates, @fallwith. Especially with the performance boost!

I think we might want a changelog entry for this. What do you think?

Sounds good. Let me figure out what's going on with these unit tests that aren't happy with the new rescue blocks and then I'll circle back with an entry that matches whatever the code that passes the tests ends up looking like.

[fallwith]

I really like these updates, @fallwith. Especially with the performance boost!
I think we might want a changelog entry for this. What do you think?

Sounds good. Let me figure out what's going on with these unit tests that aren't happy with the new rescue blocks and then I'll circle back with an entry that matches whatever the code that passes the tests ends up looking like.

Ah ok. I was rescuing too far up the chain. We want legitimate issues to be surfaced and not rescued. With the latest commit we're still rescuing all headers parsing and other determinations of whether the browser agent should be inserted. The tests should pass now and a changelog entry has been added.

[github-actions]

SimpleCov Report

	Coverage	Threshold
Line	93.73%	93%
Branch	71.85%	71%

[hannahramadan]

TIL about %r{}! Nice

[fallwith]

It's most useful for cases like this where there's a literal forward slash character involved in the pattern.

[hannahramadan]

Could do rescue => e here but as I'm also cool with it as-is!

[fallwith]

Aha! You and I were talking about this one recently and I was trying to figure out where my knowledge of a related best practice was coming from.

It turns out that RuboCop prefers it this way, with a named class. See Style/RescueStandardError. For some reason this project has (temporarily?) disabled that cop, but I still heed its advice and would prefer to stick with a named class.

[hannahramadan]

ser bra ut!