-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat(feedback): add User Feedbacks as Events #2604
Changes from 23 commits
6c717ff
aafea3e
3909c5a
a24ea5f
4af3e0c
62cf1bd
ab75fe7
a6e1631
6e80db1
f27108b
d217857
253ec29
ae47896
137f750
fac716f
bca72e7
7ac8207
d346dcf
ab93f41
307be2f
4b73892
1d5aa20
95b2176
efe9870
037c882
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,12 @@ pub enum DataCategory { | |
/// but we define it here to prevent clashing values since this data category enumeration | ||
/// is also used outside of Relay via the Python package. | ||
MonitorSeat = 13, | ||
/// User Feedback | ||
/// | ||
/// Represents a User Feedback processed. | ||
/// Currently standardized on name UserReportV2 to avoid clashing with the old UserReport. | ||
/// TODO(jferg): Rename this to UserFeedback once old UserReport is deprecated. | ||
UserReportV2 = 14, | ||
jjbayer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// | ||
// IMPORTANT: After adding a new entry to DataCategory, go to the `relay-cabi` subfolder and run | ||
// `make header` to regenerate the C-binding. This allows using the data category from Python. | ||
|
@@ -108,6 +114,7 @@ impl DataCategory { | |
Self::Monitor => "monitor", | ||
Self::Span => "span", | ||
Self::MonitorSeat => "monitor_seat", | ||
Self::UserReportV2 => "feedback", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add this string to the |
||
Self::Unknown => "unknown", | ||
} | ||
} | ||
|
@@ -157,6 +164,7 @@ impl From<EventType> for DataCategory { | |
EventType::Csp | EventType::Hpkp | EventType::ExpectCt | EventType::ExpectStaple => { | ||
Self::Security | ||
} | ||
EventType::UserReportV2 => Self::UserReportV2, | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ mod reprocessing; | |
mod response; | ||
mod runtime; | ||
mod trace; | ||
|
||
mod user_report_v2; | ||
pub use app::*; | ||
pub use browser::*; | ||
pub use cloud_resource::*; | ||
|
@@ -27,6 +27,7 @@ pub use reprocessing::*; | |
pub use response::*; | ||
pub use runtime::*; | ||
pub use trace::*; | ||
pub use user_report_v2::*; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should i include that refactor in this PR, or could I follow up with that? not sure how you all like to split changes like that into separate PRs or not. |
||
|
||
#[cfg(feature = "jsonschema")] | ||
use relay_jsonschema_derive::JsonSchema; | ||
|
@@ -67,6 +68,9 @@ pub enum Context { | |
Profile(Box<ProfileContext>), | ||
/// Information related to Replay. | ||
Replay(Box<ReplayContext>), | ||
/// Information related to User Report V2. TODO:(jferg): rename to UserFeedbackContext | ||
#[metastructure(tag = "feedback")] | ||
UserReportV2(Box<UserReportV2Context>), | ||
/// Information related to Monitors feature. | ||
Monitor(Box<MonitorContext>), | ||
/// Auxilliary information for reprocessing. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
#[cfg(feature = "jsonschema")] | ||
use relay_jsonschema_derive::JsonSchema; | ||
use relay_protocol::{Annotated, Empty, FromValue, IntoValue, Object, Value}; | ||
|
||
use crate::processor::ProcessValue; | ||
|
||
/// Feedback context. | ||
/// | ||
/// This contexts contains user feedback specific attributes. | ||
/// We don't PII scrub contact_email as that is provided by the user. | ||
/// TODO(jferg): rename to FeedbackContext once old UserReport logic is deprecated. | ||
#[derive(Clone, Debug, Default, PartialEq, Empty, FromValue, IntoValue, ProcessValue)] | ||
#[cfg_attr(feature = "jsonschema", derive(JsonSchema))] | ||
pub struct UserReportV2Context { | ||
/// The feedback message which contains what the user has to say. | ||
pub message: Annotated<String>, | ||
|
||
/// an email optionally provided by the user, which can be different from user.email | ||
#[metastructure(pii = "false")] | ||
pub contact_email: Annotated<String>, | ||
/// Additional arbitrary fields for forwards compatibility. | ||
#[metastructure(additional_properties, retain = "true")] | ||
pub other: Object<Value>, | ||
} | ||
|
||
impl super::DefaultContext for UserReportV2Context { | ||
fn default_key() -> &'static str { | ||
"feedback" | ||
} | ||
|
||
fn from_context(context: super::Context) -> Option<Self> { | ||
match context { | ||
super::Context::UserReportV2(c) => Some(*c), | ||
_ => None, | ||
} | ||
} | ||
|
||
fn cast(context: &super::Context) -> Option<&Self> { | ||
match context { | ||
super::Context::UserReportV2(c) => Some(c), | ||
_ => None, | ||
} | ||
} | ||
|
||
fn cast_mut(context: &mut super::Context) -> Option<&mut Self> { | ||
match context { | ||
super::Context::UserReportV2(c) => Some(c), | ||
_ => None, | ||
} | ||
} | ||
|
||
fn into_context(self) -> super::Context { | ||
super::Context::UserReportV2(Box::new(self)) | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use crate::protocol::Context; | ||
|
||
#[test] | ||
fn test_feedback_context() { | ||
let json = r#"{ | ||
"message": "test message", | ||
"contact_email": "test@test.com", | ||
"type": "feedback" | ||
}"#; | ||
let context = Annotated::new(Context::UserReportV2(Box::new(UserReportV2Context { | ||
message: Annotated::new("test message".to_string()), | ||
contact_email: Annotated::new("test@test.com".to_string()), | ||
other: Object::default(), | ||
}))); | ||
|
||
assert_eq!(context, Annotated::from_json(json).unwrap()); | ||
assert_eq!(json, context.to_json_pretty().unwrap()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,6 +112,8 @@ pub enum ItemType { | |
CheckIn, | ||
/// A standalone span. | ||
Span, | ||
/// UserReport as an Event | ||
UserReportV2, | ||
/// A new item type that is yet unknown by this version of Relay. | ||
/// | ||
/// By default, items of this type are forwarded without modification. Processing Relays and | ||
|
@@ -127,6 +129,7 @@ impl ItemType { | |
match event_type { | ||
EventType::Default | EventType::Error => ItemType::Event, | ||
EventType::Transaction => ItemType::Transaction, | ||
EventType::UserReportV2 => ItemType::UserReportV2, | ||
EventType::Csp | EventType::Hpkp | EventType::ExpectCt | EventType::ExpectStaple => { | ||
ItemType::Security | ||
} | ||
|
@@ -145,6 +148,7 @@ impl fmt::Display for ItemType { | |
Self::RawSecurity => write!(f, "raw_security"), | ||
Self::UnrealReport => write!(f, "unreal_report"), | ||
Self::UserReport => write!(f, "user_report"), | ||
Self::UserReportV2 => write!(f, "feedback"), | ||
Self::Session => write!(f, "session"), | ||
Self::Sessions => write!(f, "sessions"), | ||
Self::Statsd => write!(f, "statsd"), | ||
|
@@ -173,6 +177,7 @@ impl std::str::FromStr for ItemType { | |
"raw_security" => Self::RawSecurity, | ||
"unreal_report" => Self::UnrealReport, | ||
"user_report" => Self::UserReport, | ||
"feedback" => Self::UserReportV2, | ||
"session" => Self::Session, | ||
"sessions" => Self::Sessions, | ||
"statsd" => Self::Statsd, | ||
|
@@ -556,6 +561,7 @@ impl Item { | |
ItemType::Statsd | ItemType::MetricBuckets => None, | ||
ItemType::FormData => None, | ||
ItemType::UserReport => None, | ||
ItemType::UserReportV2 => None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means no outcomes will be generated. Is this intended? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for now, yes. will change this once we have data flowing through system and we're closer to alpha etc. |
||
ItemType::Profile => Some(if indexed { | ||
DataCategory::ProfileIndexed | ||
} else { | ||
|
@@ -700,7 +706,8 @@ impl Item { | |
| ItemType::Transaction | ||
| ItemType::Security | ||
| ItemType::RawSecurity | ||
| ItemType::UnrealReport => true, | ||
| ItemType::UnrealReport | ||
| ItemType::UserReportV2 => true, | ||
|
||
// Attachments are only event items if they are crash reports or if they carry partial | ||
// event payloads. Plain attachments never create event payloads. | ||
|
@@ -758,6 +765,8 @@ impl Item { | |
ItemType::RawSecurity => true, | ||
ItemType::UnrealReport => true, | ||
ItemType::UserReport => true, | ||
ItemType::UserReportV2 => true, | ||
|
||
ItemType::ReplayEvent => true, | ||
ItemType::Session => false, | ||
ItemType::Sessions => false, | ||
|
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.
Depending on when UserReport -> UserFeedback transition happens, we may need to support both alternatives to avoid future breaking changes. Have we considered this? Is there any ETA on the deprecation?
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.
Why not use
UserFeedback
right away? I agree with Iker that renaming later could bring extra complexity and possible compatibility issues.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.
when discussing with @jan-auer over initial call, he called out to not have it so there are "synonyms" in the code. I can change to UserFeedback if that is preferred.
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.
I think more important than the internal naming is making sure that the serialized string representations in the public interface do not have to be changed, e.g
"user_report_v2"
for the data category.The internal names of the types are less important, but I did a case insensitive search for "feedback" and found only a few references in comments, so I would personally go for the new naming straight away, but I'll leave it up to you. cc @jan-auer