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

Add Excon instrumentation #2383

Merged
merged 11 commits into from
Nov 19, 2024

Conversation

frederikspang
Copy link
Contributor

Description

Describe your changes:
Adds instrumentation for https://github.com/excon/excon using Excon's Middleware model.

I wanted badly to use with_child_span, however, that seems impossible with the current middleware implementation for Excon.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.17%. Comparing base (b31f0f3) to head (fee8a88).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2383      +/-   ##
==========================================
+ Coverage   98.13%   98.17%   +0.04%     
==========================================
  Files         126      128       +2     
  Lines        4767     4825      +58     
==========================================
+ Hits         4678     4737      +59     
+ Misses         89       88       -1     
Components Coverage Δ
sentry-ruby 98.56% <100.00%> (+0.05%) ⬆️
sentry-rails 97.08% <ø> (ø)
sentry-sidekiq 96.96% <ø> (ø)
sentry-resque 92.85% <ø> (ø)
sentry-delayed_job 95.65% <ø> (ø)
sentry-opentelemetry 99.31% <ø> (ø)
Files with missing lines Coverage Δ
sentry-ruby/lib/sentry-ruby.rb 100.00% <100.00%> (+0.45%) ⬆️
sentry-ruby/lib/sentry/excon.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/excon/middleware.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/utils/http_tracing.rb 100.00% <100.00%> (ø)
---- 🚨 Try these New Features:

Copy link
Collaborator

@solnic solnic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This is looking good.

Screenshot 2024-10-29 at 14 37 00 Screenshot 2024-10-29 at 14 37 52 Screenshot 2024-10-29 at 14 38 29 Screenshot 2024-10-29 at 14 38 51

sentry-ruby/lib/sentry/excon/middleware.rb Outdated Show resolved Hide resolved
@solnic
Copy link
Collaborator

solnic commented Oct 29, 2024

@frederikspang hmm seems like these failures are legit:

1) Sentry::Excon with tracing enabled with config.send_default_pii = true records the request's span with query string in data
     Failure/Error:
       expect(request_span.data).to eq({
         "http.response.status_code" => 200,
         "url" => "http://example.com/path",
         "http.request.method" => "GET",
         "http.query" => "foo=bar"
       })
     
       expected: {"http.query"=>"foo=bar", "http.request.method"=>"GET", "http.response.status_code"=>200, "url"=>"http://example.com/path"}
            got: {"http.query"=>{"foo"=>"bar"}, "http.request.method"=>"GET", "http.response.status_code"=>200, "url"=>"http://example.com/path"}
     
       (compared using ==)
     
       Diff:
       @@ -1,4 +1,4 @@
       -"http.query" => "foo=bar",
       +"http.query" => {"foo"=>"bar"},
        "http.request.method" => "GET",
        "http.response.status_code" => 200,
        "url" => "http://example.com/path",
       
     # ./spec/sentry/excon_spec.rb:84:in `block (4 levels) in <top (required)>'
     # /usr/local/rvm/gems/default/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:124:in `block in run'
     # /usr/local/rvm/gems/default/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:110:in `run'
     # /usr/local/rvm/gems/default/gems/rspec-retry-0.6.2/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # /usr/local/rvm/gems/default/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:37:in `block (2 levels) in setup'

  2) Sentry::Excon with tracing enabled with config.send_default_pii = true records breadcrumbs
     Failure/Error: expect(crumb.data[:query]).to eq("foo=bar")
     
       expected: "foo=bar"
            got: {"foo"=>"bar"}
     
       (compared using ==)
     
       Diff:
       @@ -1 +1 @@
       -"foo=bar"
       +"foo" => "bar",
       
     # ./spec/sentry/excon_spec.rb:108:in `block (4 levels) in <top (required)>'
     # /usr/local/rvm/gems/default/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:124:in `block in run'
     # /usr/local/rvm/gems/default/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:110:in `run'
     # /usr/local/rvm/gems/default/gems/rspec-retry-0.6.2/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # /usr/local/rvm/gems/default/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:37:in `block (2 levels) in setup'

Finished in 10.32 seconds (files took 1.12 seconds to load)
1069 examples, 2 failures, 1 pending

Failed examples:

rspec ./spec/sentry/excon_spec.rb:66 # Sentry::Excon with tracing enabled with config.send_default_pii = true records the request's span with query string in data
rspec ./spec/sentry/excon_spec.rb:92 # Sentry::Excon with tracing enabled with config.send_default_pii = true records breadcrumbs

this is under newer Rubies

@frederikspang
Copy link
Contributor Author

frederikspang commented Oct 29, 2024

@solnic I'm thinking this may relate to excon 1.0.0 released JUST the other day.

We've had some specs failing internally as well, post upgrade.
Like when using webmock, Host header had port before, and not after 1.0.0 release.

I'll look into this for the specs!

EDIT: Seems to be rate limit on Docker Hub right now. I'll retry tomorrow.
MIT License in Rack lets us borrow the query builder.

If I'm reading Data Model right in SDK Dev guide, data has to be one dimensional dictionary, so for :query, we'll just build it as a query-string. (And we cannot use .to_query from ActiveSupport :)

@frederikspang frederikspang force-pushed the feature/excon-instrumentation branch from e61a532 to 33e9f2d Compare November 5, 2024 15:19
@frederikspang
Copy link
Contributor Author

@solnic A few seemingly unrelated failures as far as I can tell.
Coverage should be improved to 100% patch as well.

@frederikspang frederikspang force-pushed the feature/excon-instrumentation branch from 4741cdc to bf3be51 Compare November 5, 2024 18:41
@frederikspang
Copy link
Contributor Author

@solnic Re-review? :)

@solnic
Copy link
Collaborator

solnic commented Nov 18, 2024

@frederikspang thanks! I re-tested it on top of latest master and guess what, it still works and looks good :) One last part before I can merge this in is to update this branch to latest master once more so that we can get a green build, and then I'll merge this in!

@frederikspang frederikspang force-pushed the feature/excon-instrumentation branch from bf3be51 to 4f9001b Compare November 18, 2024 10:08
@frederikspang
Copy link
Contributor Author

@solnic All good!

@solnic solnic merged commit a9b3687 into getsentry:master Nov 19, 2024
141 checks passed
@frederikspang frederikspang deleted the feature/excon-instrumentation branch November 19, 2024 15:40
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

Successfully merging this pull request may close these issues.

2 participants