Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Question] Why doesn't Sinatra instrumentation show full request url in http.target or http.route ? #411

Closed
pollosp opened this issue Apr 11, 2023 · 7 comments

Comments

@pollosp
Copy link
Contributor

pollosp commented Apr 11, 2023

Hi,
We are testing Sinatra instrumentation in one of our services and we notice that http.target or http.route or any other attribute shows the full URL requested into the client.

Looks like this is the expected behaviour according to this test here

According to semantic conventions for HTTP full url should be in http.target

http.target string The full request target as passed in a HTTP request line or equivalent.

Why is the Rack mapping removed from the http.target ? We would expect http.target in that specific test mentioned above to be /one/endpoint

Also at some point you will fill up http.app with rack mapping as said here ?

Thanks

@arielvalentin
Copy link
Collaborator

Do you have any details you can share about your setup?

Logs from OTEL_LOG_LEVEL=debug?

Screen shots of traces with missing attributes?

@pollosp
Copy link
Contributor Author

pollosp commented Apr 11, 2023

Hello , @arielvalentin I was talking mainly about your tests in this file in your code:

it 'records attributes' do
      get '/one/endpoint'

      _(exporter.finished_spans.first.attributes).must_equal(
        'http.host' => 'example.org',
        'http.method' => 'GET',
        'http.route' => '/endpoint',
        'http.scheme' => 'http',
        'http.status_code' => 200,
        'http.target' => '/endpoint'
      )
    end
    

I'm trying to understand why is this expected behaviour for the test and why is in this way to go

On the other hand as you requested, I'm able to reproduce this behaviour in my app as well (AS EXPECTED) , with the following config:

Application = Rack::Builder.app do
  map '/awesome' do
    run Awesome::Controller.new
  end
end
module Awesome
  class Controller < Sinatra::Base
      SERVICE_NAME = 'awesome'
      get '/' do
        "This is awesome!"
      end
  end
end
require "opentelemetry/instrumentation/all"

OpenTelemetry::SDK.configure do |c|
  c.use_all # enables all instrumentation!
end

This is the output of the console exporter and debug variable set:

 #<struct OpenTelemetry::SDK::Trace::SpanData
 name="GET /",
 kind=:server,
 status=
  #<OpenTelemetry::Trace::Status:0x0000ffff96c8b568 @code=1, @description="">,
 parent_span_id="\x00\x00\x00\x00\x00\x00\x00\x00",
 total_recorded_attributes=7,
 total_recorded_events=0,
 total_recorded_links=0,
 start_timestamp=1681234969445162447,
 end_timestamp=1681234969451553473,
 attributes=
  {"http.method"=>"GET",
   "http.host"=>"el-latigo:8080",
   "http.scheme"=>"http",
   "http.target"=>"",
   "http.user_agent"=>"python-requests/2.28.1",
   "http.route"=>"/",
   "http.status_code"=>200},
 links=nil,
 events=nil,
 resource=
  #<OpenTelemetry::SDK::Resources::Resource:0x0000ffff90b13380
   @attributes=
    {"service.name"=>"el-latigo",
     "process.pid"=>1,
     "process.command"=>"/usr/local/bundle/bin/puma",
     "process.runtime.name"=>"ruby",
     "process.runtime.version"=>"3.1.4",
     "process.runtime.description"=>
      "ruby 3.1.4p223 (2023-03-30 revision 957bb7cb81) [aarch64-linux]",
     "telemetry.sdk.name"=>"opentelemetry",
     "telemetry.sdk.language"=>"ruby",
     "telemetry.sdk.version"=>"1.2.0",
     "\"deployment.environment"=>"local\""}>,
 instrumentation_scope=
  #<struct OpenTelemetry::SDK::InstrumentationScope
   name="OpenTelemetry::Instrumentation::Rack",
   version="0.22.1">,
 span_id="\x0E\xCDb\xF0\x7F\xAB_\xB8",
 trace_id="\xFA\xC7\n" + "\xEFi\b \xCC\x89L\x9A\xF2Z.\xB7\xEC",
 trace_flags=#<OpenTelemetry::Trace::TraceFlags:0x0000ffff913628d8 @flags=1>,
tracestate=#<OpenTelemetry::Trace::Tracestate:0x0000ffff91366668 @hash={}> 

Gemfile for my app

gem 'aws-sdk-s3', '~> 1'
gem 'faraday', '~> 2'
gem 'faker', '~> 2'
gem 'puma', '~> 5'
gem 'rack', '~> 2'
gem 'rake', '~> 13'
gem "redis-namespace", "~> 1.8"
gem 'sentry-ruby', '~> 5'
gem 'sinatra', '~> 2'
gem 'ox', '~> 2'
gem 'opentelemetry-sdk', '~> 1'
gem 'opentelemetry-exporter-otlp', '~> 0'
gem 'opentelemetry-instrumentation-all', '~> 0'

Thanks in advance.

@pollosp
Copy link
Contributor Author

pollosp commented Apr 12, 2023

Hello again
We needed to requiere explicitly the Rack middleware to have a full http.target of request somewhere

As here:

Application = Rack::Builder.app do
  use OpenTelemetry::Instrumentation::Rack::Middlewares::TracerMiddleware
  map '/awesome' do
    run Awesome::Controller.new
  end
end

Declaring explicitly the middle ware it adds http.target as expected with the full path as first trace into a new Rack span and links a subspan for the specific Sinatra controller , so you can search in your Opentelemetry backend by the full http.target in jaegger for example.

Screenshot 2023-04-12 at 12 28 13
Screenshot 2023-04-12 at 12 28 21
Screenshot 2023-04-12 at 12 28 43 2

I expected as I told you having by default http.target filled with the full URL in this case /awesome instead / just using the Sinatra instrumentation without declaring explicitly the Rack middleware into the Rack builder.

Don't know if this is intentional or not. as looks like you are picking explicitly sinatra.route and the test are expecting it instead REQUEST_URI which has the full URI into the Sinatra tracer context.

  • What I understand here is Sinatra instrumentation is braking somehow http.target the semantic convention
    • Maybe I understood it wrongly
  • Looks like Rack instrumentation declaring explicitly the middleware follow the semantic convention
  • Readme you provide for Sinatra and Rack instrumentation aren't clear enough or detailed for me for me (Readme Docs).
    • Into the Rack instrumentation examples it is more clear you need to require the Middelware which at the makes sense, but I didn't see it into the Readme
  • As well as you can see into the screenshots Library otel.library.name is always Rack , maybe when it reach Sinatra Trace it should mutate to otel.library.name Sinatra making clear that trace come from this specific instrumentation library

Thanks again for your time

@arielvalentin
Copy link
Collaborator

We needed to requiere explicitly the Rack middleware to have a full http.target of request somewhere

Yes that is the case. Are you manually configuring the instrumentations in the SDK? If so order does matter since the sinatra instrumentation depends on Rack.

We have seen similar reports with ActionPack: #88

Don't know if this is intentional or not. as looks like you are picking explicitly sinatra.route and the test are expecting it instead REQUEST_URI which has the full URI into the Sinatra tracer context.

I cannot say for sure for every decision. In a lot of cases the code and tests were copied from dd-trace-rb so it could be incorrect.

Looks like Rack instrumentation declaring explicitly the middleware follow the semantic convention
Readme you provide for Sinatra and Rack instrumentation aren't clear enough or detailed for me for me (Readme Docs).

We need a lot of help updating documentation! If you and your team are available to help with docs and maintenance of the sinatra instrumentation we will be grateful!

Into the Rack instrumentation examples it is more clear you need to require the Middelware which at the makes sense, but I didn't see it into the Readme
As well as you can see into the screenshots Library otel.library.name is always Rack , maybe when it reach Sinatra Trace it should mutate to otel.library.name Sinatra making clear that trace come from this specific instrumentation library

Tracers have immutable Instrumentation Scopes (otel.library.name), which means you will not be able to change this at runtime. We chose reuse/enrich the Rack span which has its own tracer as opposed to adding a Sinatra specific span with duplicate attributes.

Would you prefer to have separate spans for Sinatra or not rely on Rack dependency?

cc: @scbjans @rdoherty-fastly @robertlaurin Do you all have any insights that you may be able to share?

@pollosp
Copy link
Contributor Author

pollosp commented Apr 12, 2023

Yes that is the case. Are you manually configuring the instrumentations in the SDK? If so order does matter since the sinatra instrumentation depends on Rack.

We are using:

gem 'opentelemetry-sdk', '~> 1'
gem 'opentelemetry-exporter-otlp', '~> 0'
gem 'opentelemetry-instrumentation-all', '~> 0'

And we inicialice OTEL in this way:

require "opentelemetry/sdk"
require "opentelemetry/exporter/otlp"
require "opentelemetry/instrumentation/all"

OpenTelemetry::SDK.configure do |c|
  c.use_all # enables all instrumentation!
end

Would you prefer to have separate spans for Sinatra or not rely on Rack dependency?

I think will be easier but not strong opinion still starting with OTEL

We need a lot of help updating documentation! If you and your team are available to help with docs and maintenance of the sinatra instrumentation we will be grateful!

Sure I will help you :) , working on the EasyCLA now

@scbjans
Copy link
Contributor

scbjans commented Apr 12, 2023

Not sure if I add something new to the conversation:

The Sinatra instrumentation uses the current Rack span and enriches it with Sinatra attributes. A benefit from that is that we can now piggyback on Racks configuration options (e.g untraced endpoints).
This is indeed all quite undocumented and any help - documenting or otherwise - would be greatly appreciated.

From the issue @arielvalentin mentioned: an example of setting untraced endpoints is given in the comment.

I'll also try to look into some documentation efforts next week.

@pollosp
Copy link
Contributor Author

pollosp commented Apr 13, 2023

Thanks all I think it is clear for me I will try to contribute to the documentation as well.
Closing the issue from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants