Sinatra and Slim: Option :sky_virtual_path is invalid about skylight-ruby

[wagenet]

No problem. I can definitely imagine that our tests aren't paying attention to errors in the logs, just whether the end result is successful.

[chancancode]

This ticket was quite old, @benalavi what version of the agent and dependencies are you using? If you can send a copy of your Gemfile.lock to [email protected] (or via the in-app messenger), that would be quite helpful. We have overhauled the implementation of the agent quite significantly since this was originally reported, so it may be a different issue if you are still seeing this in a recent version of the agent.

[benalavi]

Here is a minimal test case:

Gemfile

source "https://rubygems.org"
ruby "3.0.1"

gem "rack"
gem "puma"
gem "sinatra"
gem "tilt"
gem "haml"
gem "skylight"

config.ru

# frozen_string_literal: true

require "sinatra/base"
require "skylight/sinatra"

Skylight.start! file: "skylight.yml"

class Test < Sinatra::Base
  get "/" do
    haml ""
  end
end

run Test

skylight.yml

# otherwise says empty file and won't start
ignored_endpoints:

---

Gemfile.lock

GEM
  remote: https://rubygems.org/
  specs:
    activesupport (6.1.3.1)
      concurrent-ruby (~> 1.0, >= 1.0.2)
      i18n (>= 1.6, < 2)
      minitest (>= 5.1)
      tzinfo (~> 2.0)
      zeitwerk (~> 2.3)
    concurrent-ruby (1.1.8)
    haml (5.2.1)
      temple (>= 0.8.0)
      tilt
    i18n (1.8.10)
      concurrent-ruby (~> 1.0)
    minitest (5.14.4)
    mustermann (1.1.1)
      ruby2_keywords (~> 0.0.1)
    nio4r (2.5.7)
    puma (5.2.2)
      nio4r (~> 2.0)
    rack (2.2.3)
    rack-protection (2.1.0)
      rack
    ruby2_keywords (0.0.4)
    sinatra (2.1.0)
      mustermann (~> 1.0)
      rack (~> 2.2)
      rack-protection (= 2.1.0)
      tilt (~> 2.0)
    skylight (5.0.1)
      activesupport (>= 5.2.0)
    temple (0.8.2)
    tilt (2.0.10)
    tzinfo (2.0.4)
      concurrent-ruby (~> 1.0)
    zeitwerk (2.4.2)

PLATFORMS
  x86_64-darwin-18

DEPENDENCIES
  haml
  puma
  rack
  sinatra
  skylight
  tilt

RUBY VERSION
   ruby 3.0.1p64

BUNDLED WITH
   2.2.15

Run commands:

SKYLIGHT_AUTHENTICATION=... bundle exec rackup

curl 127.0.0.1:9292
curl 127.0.0.1:9292

Server output:

Puma starting in single mode...
* Puma version: 5.2.2 (ruby 3.0.1-p64) ("Fettisdagsbulle")
*  Min threads: 0
*  Max threads: 5
*  Environment: development
*          PID: 97390
* Listening on http://127.0.0.1:9292
* Listening on http://[::1]:9292
Use Ctrl-C to stop
Haml::TempleEngine: Option :sky_virtual_path is invalid
127.0.0.1 - - [29/Apr/2021:19:06:55 -0700] "GET / HTTP/1.1" 200 - 0.0447
Haml::TempleEngine: Option :sky_virtual_path is invalid
127.0.0.1 - - [29/Apr/2021:19:06:58 -0700] "GET / HTTP/1.1" 200 - 0.0031

In our case we render quite a few haml templates in some of our views and so we get tons of Haml::TempleEngine: Option :sky_virtual_path is invalid lines emitted for each route. I assume it is happening each time haml is called.

We also just found that Haml::TempleEngine.disable_option_validator! stops the log lines from being emitted, and is probably a better idea than filtering them out of our log collector.

[chancancode]

Thanks for the reproduction and workaround. Sorry for the trouble!

It seems like the problem is due to this option being added here:

skylight-ruby/lib/skylight/probes/sinatra.rb

Lines 42 to 44 in a312f9d

	options[:sky_virtual_path] = data.is_a?(Symbol) ? data.to_s : "Inline template (#{engine})"
	super

We'll look into refactoring this to avoid the warning. In the meantime, Haml::TempleEngine.define_options(:sky_virtual_path) is a more precise workaround to silence the warning, and should be harmless.

[chancancode]

Here is a modified version of the self-contained reproduction:

# config.ru

# run with SKYLIGHT_AUTHENTICATION=... rackup

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  ruby "3.0.1"

  gem "pry"
  gem "rack"
  gem "puma"
  gem "sinatra"
  gem "tilt"
  gem "haml"
  gem "skylight"
end

require "skylight/sinatra"

# Haml::TempleEngine.define_options(:sky_virtual_path)

Skylight.start!

class Test < Sinatra::Base
  get "/" do
    haml ""
  end
end

[wycats]

@runar we're passing an extra option to the template engine that we use later to track what's happening in Sinatra. Slim has decided to add an extra layer of validation, which causes this extra option to throw an error, because the authors of Slim assumed that passing an unknown option is an error.

I'm currently on vacation but I'll try to take a look at an alternative mitigation that we can apply automatically in Skylight when I get back.

[runar]

@wycats Thank you so much for your fast and useful response. I’ll continue with disabling the option validator for now. Enjoy your vacation! 😃

[hakusaro]

Thanks for the still-timely workaround, @runar c:

[wagenet]

@runar @hakusaro do either of you happen to know if this is still an issue?

[hakusaro]

Not off the top of my head. I would assume that unless you've targeted this issue directly it's still a problem.

Sinatra is a pretty tiny framework. You should just be able to setup Skylight and a basic get request with the Slim renderer and reproduce it though.

[wagenet]

@hakusaro thanks for the quick reply. I'll try to do this next time I have a chance.

[hakusaro]

I'll let you know if I can beat you to it. It might be a more-complicated setup than just trivial Sinatra + slim, but I want to double check that before getting back to you.

[benalavi]

I think we're seeing this same issue with Sinatra + HAML. Specifically Haml::TempleEngine: Option :sky_virtual_path is invalid. In our case we're now filtering it back out on the way into our log storage but it was generating quite a bit of log volume for a bit.

[zvkemp]

@benalavi The current master branch (on rubygems as 5.1.0.beta2) mitigates this issue by not passing the custom option.