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

Spotlight Sidecar support in Sentry-Ruby #2175

Merged
merged 8 commits into from
Dec 5, 2023

Conversation

natikgadzhi
Copy link
Contributor

@natikgadzhi natikgadzhi commented Nov 23, 2023

Summary

This PR adds Spotlight / Sidecar support to Sentry-Ruby!

This is still work in progress ;) /cc @sl0thentr0py, @cleptric, feedback very welcome!

Changes

  • Added a separate Spotlight::Configuration object that has enabled and sidecar_url fields with defaults.
  • Added a separate Spotlight::Transport that is similar to HTTPTransport, but YOLO-grade. Doesn't care about rate limits.
  • Added a hook into http_transport.rb (not proud of that one) that calls Spotlight::Transport.new().send_data(data) when it has stuff to send, but before validating rate limits. Aligned with PHP version.

TODO

Reviewing

Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Merging #2175 (5e8462d) into master (9a22f26) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2175   +/-   ##
=======================================
  Coverage   97.34%   97.34%           
=======================================
  Files          99      100    +1     
  Lines        3692     3731   +39     
=======================================
+ Hits         3594     3632   +38     
- Misses         98       99    +1     
Components Coverage Δ
sentry-ruby 98.02% <100.00%> (-0.01%) ⬇️
sentry-rails 94.98% <ø> (ø)
sentry-sidekiq 94.53% <ø> (ø)
sentry-resque 93.65% <ø> (ø)
sentry-delayed_job 94.44% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
sentry-ruby/lib/sentry/client.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/configuration.rb 98.74% <100.00%> (+0.01%) ⬆️
sentry-ruby/lib/sentry/transport.rb 99.01% <100.00%> (-0.04%) ⬇️
sentry-ruby/lib/sentry/transport/http_transport.rb 100.00% <100.00%> (ø)
...y-ruby/lib/sentry/transport/spotlight_transport.rb 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@HazAT
Copy link
Member

HazAT commented Nov 27, 2023

@natikgadzhi Added support to the sidecar to accept gzip/deflate encoding

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

Generally LGTM - since this is a dev only feature and opt-in, I think we are fine with the tests we have but @sl0thentr0py should make sure this is how we want it to be
I already documented it here
https://spotlightjs.com/setup/sentry/

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Nov 27, 2023 via email

@natikgadzhi
Copy link
Contributor Author

Okay, update time!

  • I removed the separate Spotlight::Configuration in favor of the simple Sentry-config-level setting.
  • I verified that the thing actually works.

As long as @sl0thentr0py is happy, we can merge this as is, so folks can start using it. My approach would be to merge and follow-up with next steps, but while I have time, I'll add things in this PR:

  • Improve Spotlight transport layer — try to re-use HTTPTransport as much as possible, perhaps inherit from it and override the methods that we won't use explicitly.
  • Add tests for Spotlight itself to verify it picks up config URL correctly.
  • Add back gzip encoding (perhaps easiest to inherit from http_transport).

/cc @HazAT

@natikgadzhi
Copy link
Contributor Author

@HazAT, thank you for the Spotlight docs!

@sl0thentr0py
Copy link
Member

taking this over to finish up

@sl0thentr0py
Copy link
Member

tested new changes without DSN

image image

Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

ok i made some changes and added specs
@natikgadzhi @HazAT if you wanna take a quick look, otherwise i'll merge and release tomorrow

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Dec 4, 2023 via email

@sl0thentr0py
Copy link
Member

discussed with @HazAT - i will add a max 3 retry on the spotlight transport so we don't spam logs in production tomorrow

@sl0thentr0py sl0thentr0py marked this pull request as draft December 4, 2023 16:50
@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Dec 4, 2023 via email

@sl0thentr0py
Copy link
Member

i'm not sure if spotlight docs will be in sentry or separate, leaving that to @HazAT

@sl0thentr0py sl0thentr0py marked this pull request as ready for review December 5, 2023 14:03
@sl0thentr0py sl0thentr0py force-pushed the ng/feat/spotlight branch 2 times, most recently from 4ac6613 to c83f571 Compare December 5, 2023 14:41
@Lms24
Copy link
Member

Lms24 commented Dec 5, 2023

@sl0thentr0py sl0thentr0py merged commit a4063d1 into getsentry:master Dec 5, 2023
97 checks passed
st0012 pushed a commit that referenced this pull request Dec 6, 2023
The link should point to <#2175> (not "pulls").
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.

None yet

4 participants