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

Correct type attribute's usages #1354

Merged
merged 3 commits into from
Mar 26, 2021
Merged

Correct type attribute's usages #1354

merged 3 commits into from
Mar 26, 2021

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Mar 21, 2021

Based on this discussion, normal events shouldn't have the type attribute and only transaction events should have type: "transaction".

So this PR:

  • removes type: "event" from normal events
  • corrects Transport's envelope generation logic: envelope's type is not equivalent to event's type
  • corrects the logging message for sending envelopes

@st0012 st0012 added this to the 4.4.0 milestone Mar 21, 2021
@st0012 st0012 self-assigned this Mar 21, 2021
@st0012 st0012 added this to In progress in 4.x via automation Mar 21, 2021
@st0012 st0012 requested a review from jan-auer March 21, 2021 07:58
@codecov-io
Copy link

codecov-io commented Mar 21, 2021

Codecov Report

Merging #1354 (4368fda) into master (351c64e) will increase coverage by 0.59%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1354      +/-   ##
==========================================
+ Coverage   98.08%   98.68%   +0.59%     
==========================================
  Files         208      113      -95     
  Lines        9162     5327    -3835     
==========================================
- Hits         8987     5257    -3730     
+ Misses        175       70     -105     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/event.rb 98.76% <ø> (-0.02%) ⬇️
sentry-ruby/spec/sentry/transport_spec.rb 100.00% <ø> (ø)
sentry-rails/spec/sentry/send_event_job_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/client.rb 97.10% <100.00%> (+0.04%) ⬆️
sentry-ruby/lib/sentry/transaction_event.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/transport.rb 97.22% <100.00%> (-0.34%) ⬇️
sentry-ruby/spec/sentry/rake_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/core_ext/object/deep_dup.rb 93.75% <0.00%> (-6.25%) ⬇️
...-raven/spec/raven/integrations/rails/event_spec.rb
sentry-raven/spec/raven/transports/http_spec.rb
... and 93 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 351c64e...4368fda. Read the comment docs.

@jan-auer
Copy link
Member

Adding @rhcarvalho to double-check this.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Looks good to me, one minor suggestion below to clarify on the item type.

sentry-ruby/lib/sentry/transport.rb Outdated Show resolved Hide resolved
4.x automation moved this from In progress to Reviewer approved Mar 22, 2021
@st0012 st0012 modified the milestones: 4.4.0, sentry-ruby-4.3.2 Mar 25, 2021
Normal events shouldn't have the `type` field. Only transactions should
have `type: "transaction"`.

But in the logs, we should still print normal events' type as `"event"`.

For more information about the rules, please see https://github.com/getsentry/sentry-ruby/pull/1336/files#r596851969
@st0012 st0012 merged commit 86dcf3c into master Mar 26, 2021
@st0012 st0012 deleted the correct-event-type branch March 26, 2021 10:21
4.x automation moved this from Reviewer approved to Done Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.x
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants