Untraced endpoints in Rack-based instrumentations

[bolandoboy]

Hi! In order to silence the instrumentation of some endpoints in our applications we're doing something similar to this minimal example. This is Sinatra, but we're doing the same with Grape and Rails/ActionPack, all of them using the Rack instrumentation under the hood.

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "opentelemetry-sdk"
  gem "opentelemetry-instrumentation-sinatra"
  gem "sinatra"
  gem "thin"
end

require "opentelemetry-sdk"
require "sinatra/base"

ENV["OTEL_TRACES_EXPORTER"] = "console"

OpenTelemetry::SDK.configure do |c|
  c.use "OpenTelemetry::Instrumentation::Rack", {untraced_endpoints: ["/health/check"]}
  c.use "OpenTelemetry::Instrumentation::Sinatra"
end

class App < Sinatra::Application
  get "/payment" do
    "Here's your payment...\n"
  end

  get "/health/check" do
    "All good!\n"
  end
end

Rack::Handler::Thin.run App

We haven't seen this documented, but we assumed we could do it, and it certainly works for this example... But then we ran into some problems with more complex applications.

So, before going into maybe unnecessary details, is this something supported or it's a hack taking advantage of knowing that it's all Rack under all the web framework instrumentations?

[arielvalentin]

Yes, the untraced_endpoints feature is something we support, however I would encourage you to consider using your tracing infrastructure to filter out health check traces (e.g. otel-collector) or see if you are able to configure the health check system emit traces flags that disable sampling.

untraced_endpoints is not as feature rich as the collector and may also result in crucial missing data.

A few use cases I can think of are where you see that your orchestrator has taken a container out of service for failing to respond to health checks or a burst of health check requests from a bad actor.

If your app uses health checks to test or ping other services like DB pool health, redis connections etc... those spans won't be visible to you and it many be critical in debugging what is happening.

[bolandoboy]

Thanks for your two-fold response. Re: health checks, I totally agree. Traditionally we've silenced them because the signal/noise ratio was very low, but thanks to OTEL we'll be able to provide much richer information and we're definitely going to push teams to instrument properly. Thanks for the feedback, having "official word" on this will be helpful for us.

All that said, going back to untraced_endpoints and using /health/check just as an example, we've found the following "problem". Using quotes because we don't know if it's intended behaviour.

Running curl localhost:8080/health/check in the above example will not trace the endpoint, which is correct, but if we build our Rack application like this, it will trace it.

Notice the relevant part is this:

App = Rack::Builder.new do
  map "/health" do
    run HealthController
  end
  ...
end

Full example:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "opentelemetry-sdk"
  gem "opentelemetry-instrumentation-sinatra"
  gem "sinatra"
  gem "thin"
end

require "opentelemetry-sdk"
require "sinatra/base"

ENV["OTEL_TRACES_EXPORTER"] = "console"

OpenTelemetry::SDK.configure do |c|
  c.use "OpenTelemetry::Instrumentation::Rack", {untraced_endpoints: ["/health/check"]}
  c.use "OpenTelemetry::Instrumentation::Sinatra"
end

class Endpoints < Sinatra::Application
  get "/payment" do
    "Here's your payment...\n"
  end
end

class HealthController < Sinatra::Application
  get "/check" do
    "All good!\n"
  end
end

App = Rack::Builder.new do
  map "/health" do
    run HealthController
  end

  run Endpoints
end

Rack::Handler::Thin.run App

In this scenario, if we want to silence /health/check with the current Rack instrumentation, we have to do untraced_endpoints: ["/check"], which we find somewhat problematic for a few of reasons:

1. It's counterintuitive, since the URL clients use is still /health/check.
2. You're a refactor of your mappings/controllers away from breaking the OTEL config.
3. It may conflict with other endpoints, as in the following example where there's no way to silence /health/check without silencing /payment/check. Notice it's 2 different Sinatra apps with the same endpoint mapped to different Rack paths.

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "opentelemetry-sdk"
  gem "opentelemetry-instrumentation-sinatra"
  gem "sinatra"
  gem "thin"
end

require "opentelemetry-sdk"
require "sinatra/base"

ENV["OTEL_TRACES_EXPORTER"] = "console"

OpenTelemetry::SDK.configure do |c|
  c.use "OpenTelemetry::Instrumentation::Rack", {untraced_endpoints: ["/check"]}
  c.use "OpenTelemetry::Instrumentation::Sinatra"
end

class PaymentsController < Sinatra::Application
  get "/check" do
    "This smells fishy...\n"
  end
end

class HealthController < Sinatra::Application
  get "/check" do
    "All good!\n"
  end
end

App = Rack::Builder.new do
  map "/payment" do
    run PaymentsController
  end

  map "/health" do
    run HealthController
  end
end

Rack::Handler::Thin.run App

We're making these scenarios work by using untraced_requests instead of untraced_endpoints, using this lambda in our config of Sinatra, Rails and ActionPack:

->(rack_env) do
  rack_mapping = (rack_env["SCRIPT_NAME"] || "").delete_suffix("/")
  path_info = (rack_env["PATH_INFO"] || "").delete_suffix("/")

  untraced_endpoints.include?(rack_mapping + path_info)
end

Would it make sense to add this to the base Rack instrumentation, so it takes into account that map "/whatever" will show up in the Rack env as env["SCRIPT_NAME"] -> "/whatever"?

[bolandoboy]

Sorry about the lengthy comment above. Going forward I don't know if you guys would prefer more straight-to-the-point code snippets or fully functional executable examples.

[arielvalentin]

I see what you mean! It does look like this is something I would have expected to work when using this feature.

Thanks for sharing the possible solution/workaround as well and I would absolutely welcome any help in addressing issues like this.

Would you be amenable to opening a PR to handle this scenario?

[bolandoboy]

Sure! I'll see how the CLA process works and I'll get to it next week.

[arielvalentin]

@robbkidd Are you planning on working on this? Or did you assign yourself here to participate in the conversation?

[robbkidd]

It's mostly the second (I'm participating) and willingness towards the first (working on it). I currently plop my face as an assignee on issues and PRs here so that they appear associated with me on the GitHub project board I add them to for aggregating all the things I'm keeping tabs on across several dozen GitHub repos. I can see how assigning myself is not clearly communicating my intent.

[bolandoboy]

I finally managed to work on this and I think the scope of this issue is wider than I anticipated.

Long story short, in the example above mapping endpoints with Rack::Builder, it's confusing that you can untrace the endpoint /health/check while the http.target for that endpoint is just /check.

Even the instrumentation tests for the untrace feature rely on http.target, and that's what got me thinking, because the new tests didn't make any sense. They read like "configure /health/check as untraced, run the request, expect that no span has been generated for the http.target /check"

The problem is related to what we encountered in #411. In that case we decided to "fix" it in our wrapper around the Sinatra instrumentation, but now I think this is a Rack concern.

Do you people think this merits opening an issue about the value of http.target (actually outdated, now it should be url.path) and parking this one until that one is discussed?

[github-actions]

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.