-
-
Notifications
You must be signed in to change notification settings - Fork 484
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 inaccurate event model relationships #1777
Conversation
class Event | ||
TYPE = "event" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sl0thentr0py By giving all events a default "event"
type, I think we can remove many fallback code that deals with nil
type. I think the events can still be accepted without any issue. Let me know if that'll cause any problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yea that's perfect, this always felt like a code smell
@@ -71,8 +69,6 @@ def initialize(configuration:, integration_meta: nil, message: nil) | |||
@rack_env_whitelist = configuration.rack_env_whitelist | |||
|
|||
@message = (message || "").byteslice(0..MAX_MESSAGE_SIZE_IN_BYTES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, message
should be moved to ErrorEvent
as well. But since that'll require changing Event#initialize
as well and doesn't provide much benefit. I decided not to go this far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice and clean! don't see any obvious problems
Since all event will now have "event" type by default, the fallback logics aren't needed anymore.
As described in #1776 and discussed in #1773, error events shouldn't be the parent of transaction events. They're both variants of the core event type with their unique attributes. So this PR corrects the currently inaccurate inheritance relationship.
Close #1776