feat: Add client reports by sl0thentr0py · Pull Request #1604 · getsentry/sentry-ruby

sl0thentr0py · 2021-11-05T14:45:09Z

This PR adds support for Client Reports.

new config send_client_reports, true by default
record lost events in Transport
attach them to an existing envelope while sending, max 1 per 30 seconds
- PS: this differs from python which needs to do extra scheduling when it sends events via the store endpoint

Some notes:

I used minitest before, so if I did something incorrect while writing the specs, please let me know :)
I still want to test this a bit more manually since it's my first PR, so making draft for now

Transport should only care about sending the event object to the right destination with right format. It doesn't need to check if it's allowed to send an event, which should already be done by `Client#capture_event`.

* add `data_category` to closer match upstream definitions and use it in rate limit as well * record `:ratelimit_backoff` dropped events in `discarded_events`

…pped events

…nto client-reports

st0012

Generally looks good. Just some suggestions on styling and testing 👍

sentry-ruby/lib/sentry/transport.rb

sentry-ruby/spec/sentry/client/event_sending_spec.rb

sentry-ruby/spec/sentry/transaction_spec.rb

sentry-ruby/spec/sentry/transport_spec.rb

sentry-ruby/lib/sentry/transport.rb

iker-barriocanal

Didn't review the PR in-depth but overall looks good to me. I only have one question: we can read the following in the docs

[...] to periodically flush them out as separate envelope item or attach it to an already scheduled envelope.

If no envelope is sent for a long time (say, 5 minutes), does the SDK wait for the next envelope to add the client reports, or reports them before that (in an envelope with potentially only client reports)?

st0012 · 2021-11-08T12:13:50Z

@sl0thentr0py do you think this can go out this week?

sl0thentr0py · 2021-11-08T12:16:17Z

hmm I can finish making the changes and testing by tomorrow I think, so unless I find some problems, I'd say yes?

st0012 · 2021-11-08T12:17:39Z

@sl0thentr0py awesome. I'm thinking about releasing version 4.8.0 with this feature this week (align with RubyConf).

sl0thentr0py · 2021-11-08T13:21:15Z

@iker-barriocanal
short answer: no it doesn't send it's own envelope currently, it is always attached to an existing envelope.

long answer:
If the actual requirement is to always ensure 1 client report per minute, that's not so trivial because you would need some coordinating parent process that ensures this scheduling logic. AFAICT, none of the sdks implement this in that sense.

What the other 2 sdks do:

js only flushes them on browser tab state changing (visibilitychange)
python adds them to an existing envelope once every 30 seconds but also schedules one every 60 seconds

However, this 60 second scheduling is itself done only in capture_event and capture_envelope which implies that you eventually need some event/transaction to be fired to schedule this 'client report only' envelope.

Another difference: python has both capture_event which goes to the store endpoint and capture_envelope which goes to the envelope endpoint, hence the need for the separate envelope scheduling. Ruby sends everything to envelope anyway so I'm not sure we need to schedule the other one at all.

For now, considering my current understanding of what we need client reports for, I think this implementation for ruby is fine. Let me know if you have questions.

iker-barriocanal · 2021-11-08T13:38:11Z

@sl0thentr0py sure, I was just asking the question, LGTM as it is. The only thing I'd add to this conversation is to explicitly mention it somewhere, and IMO the short answer in the PR description should do it (or be more explicit on the third point).

sentry-ruby/lib/sentry/transport.rb

codecov-commenter · 2021-11-08T14:26:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.49%. Comparing base (c4d2623) to head (4a812c2).
⚠️ Report is 705 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1604      +/-   ##
==========================================
+ Coverage   98.45%   98.49%   +0.04%     
==========================================
  Files         131      131              
  Lines        7243     7322      +79     
==========================================
+ Hits         7131     7212      +81     
+ Misses        112      110       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sl0thentr0py · 2021-11-09T12:24:05Z

Tested the sample rate case end-to-end, looks good to me

sl0thentr0py · 2021-11-10T12:00:10Z

sentry-ruby/spec/spec_helper.rb

    skip("skip rack related tests") unless defined?(Rack)
  end
+
+  RSpec::Matchers.define :have_recorded_lost_event do |reason, type|


@st0012 not an rspec pro so not sure if this is the right way to do this? let me know :)

I think it looks good and I've also tested it locally 👍

sl0thentr0py

@st0012 changed the specs and made some minor changes based on various comments above, can you take it for another spin?

st0012

Thanks for the amazing feature 👍

sl0thentr0py · 2021-11-11T11:03:15Z

@st0012 thank you for helping me with my first PR!

sl0thentr0py and others added 16 commits October 29, 2021 13:39

Add send_client_reports config, true by default

9ddcc01

Remove unnecessary sending check from Transport

c2be79e

Transport should only care about sending the event object to the right destination with right format. It doesn't need to check if it's allowed to send an event, which should already be done by `Client#capture_event`.

Remove unused @configuration from Transport

3dbda5c

Remove sampling from general sending restriction logic

adc7685

Check if the event is sampled in Client#capture_event

3d55ef2

Update changelog

915a105

Implement record_lost_event in transport

5dd3e16

* add `data_category` to closer match upstream definitions and use it in rate limit as well * record `:ratelimit_backoff` dropped events in `discarded_events`

Remove data_category changes, stick to event type while recording dro…

8a41b76

…pped events

Merge remote-tracking branch 'origin/fix-event-sending-restriction' i…

cf68909

…nto client-reports

Bring back configuration attr in transport

712a7a6

Record sampling based lose events

5019d36

Record queue_overflow event drop

fa0893d

Append client report to existing envelope max once a minute

9fd99a8

Merge remote-tracking branch 'origin/master' into client-reports

006a075

Fix bad merge

56098fb

Add specs for client reports

4d35a6a

sl0thentr0py requested review from a team, iker-barriocanal, lobsterkatie and st0012 and removed request for a team November 5, 2021 14:45

sl0thentr0py added 2 commits November 5, 2021 15:47

Add changelog entry

a2a99bb

Fix config docstring

e922674

st0012 reviewed Nov 5, 2021

View reviewed changes

iker-barriocanal reviewed Nov 8, 2021

View reviewed changes

AbhiPrasad reviewed Nov 8, 2021

View reviewed changes

sentry-ruby/lib/sentry/transport.rb Outdated Show resolved Hide resolved

sentry-ruby/lib/sentry/transport.rb Outdated Show resolved Hide resolved

sl0thentr0py added 4 commits November 8, 2021 14:44

Make interval 30 seconds

df046d0

Remove configuration object from transport

1af19fc

Validate client report reasons

3787dfa

Remove configuration accessor

d9ec041

st0012 added this to the 4.8.0 milestone Nov 8, 2021

st0012 added enhancement sentry-ruby labels Nov 8, 2021

st0012 assigned sl0thentr0py Nov 8, 2021

Fix nil interpolation

e5f91b6

sl0thentr0py added 2 commits November 10, 2021 12:54

New matcher for checking recorded lost event

2e2163d

Merge remote-tracking branch 'origin/master' into client-reports

affbd6e

sl0thentr0py commented Nov 10, 2021

View reviewed changes

sl0thentr0py requested a review from st0012 November 10, 2021 12:01

sl0thentr0py marked this pull request as ready for review November 10, 2021 12:02

Update config docstring

4a812c2

st0012 approved these changes Nov 11, 2021

View reviewed changes

st0012 merged commit d654a60 into master Nov 11, 2021

st0012 deleted the client-reports branch November 11, 2021 10:58

Uh oh!

Conversation

sl0thentr0py commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st0012 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iker-barriocanal left a comment

Choose a reason for hiding this comment

Uh oh!

st0012 commented Nov 8, 2021

Uh oh!

sl0thentr0py commented Nov 8, 2021

Uh oh!

st0012 commented Nov 8, 2021

Uh oh!

sl0thentr0py commented Nov 8, 2021

Uh oh!

iker-barriocanal commented Nov 8, 2021

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Nov 8, 2021 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sl0thentr0py commented Nov 9, 2021

Uh oh!

sl0thentr0py Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

st0012 Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

sl0thentr0py left a comment

Choose a reason for hiding this comment

Uh oh!

st0012 left a comment

Choose a reason for hiding this comment

Uh oh!

sl0thentr0py commented Nov 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sl0thentr0py commented Nov 5, 2021 •

edited

Loading

codecov-commenter commented Nov 8, 2021 •

edited by codecov bot

Loading