Skip to content

Add Excon instrumentation#2383

Merged
solnic merged 11 commits intogetsentry:masterfrom
frederikspang:feature/excon-instrumentation
Nov 19, 2024
Merged

Add Excon instrumentation#2383
solnic merged 11 commits intogetsentry:masterfrom
frederikspang:feature/excon-instrumentation

Conversation

@frederikspang
Copy link
Copy Markdown
Contributor

@frederikspang frederikspang commented Aug 20, 2024

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.

@frederikspang frederikspang mentioned this pull request Aug 27, 2024
Merged
1 task
@frederikspang frederikspang force-pushed the feature/excon-instrumentation branch from 6d13941 to cafeac9 Compare October 22, 2024 15:34
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 22, 2024 •
edited
Loading

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:

solnic
solnic approved these changes Oct 29, 2024
View reviewed changes
Copy link
Copy Markdown
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

@solnic
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

frederikspang commented Oct 29, 2024 •
edited
Loading

@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
Copy Markdown
Contributor Author

frederikspang commented Nov 5, 2024

@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
Copy Markdown
Contributor Author

frederikspang commented Nov 15, 2024

@solnic Re-review? :)

@solnic
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

frederikspang commented Nov 18, 2024

@solnic All good!

@solnic solnic merged commit a9b3687 into getsentry:master Nov 19, 2024
@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

Reviewers

@solnic solnic solnic approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@frederikspang @solnic