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

feat(py): Expose dynamic rule condition validation #941

Merged
merged 8 commits into from
Mar 3, 2021

Conversation

RaduW
Copy link
Contributor

@RaduW RaduW commented Feb 26, 2021

Added dynamic rule condition validation function in sentry-relay.

#skip-changelog

@RaduW RaduW requested a review from jan-auer February 26, 2021 17:58
jan-auer
jan-auer previously approved these changes Feb 26, 2021
@jan-auer jan-auer dismissed their stale review February 26, 2021 18:41

Sorry, clicked the wrong button.

py/sentry_relay/processing.py Outdated Show resolved Hide resolved
py/tests/test_processing.py Outdated Show resolved Hide resolved
relay-cabi/src/processing.rs Outdated Show resolved Hide resolved
let ret_val = match serde_json::from_str::<RuleCondition>((*value).as_str()) {
Ok(condition) => {
if condition.supported() {
"".to_string()
Copy link
Member

Choose a reason for hiding this comment

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

You can return RelayStr::new("") here. No allocation required.

Copy link
Contributor Author

@RaduW RaduW Mar 1, 2021

Choose a reason for hiding this comment

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

I know, but I don't like it.
If you insist I'll do it but here are my reasons not to:

What I want to do (in a future PR) is change the current RealyStr::new(s: &str) to:

   pub(crate) fn new(s: &'static str) -> RelayStr 

and

   pub(crate) fn unsafe unsafe_new(s: &str) -> RelayStr 

That also affects the implementation of impl<'a> From<&'a str> for RelayStr so I need to look
a bit more in detail at it.

Until then I consider the implementation of RealyStr::new as broken, it is unsafe and breaks the Rust
guarantees. It will happily compile something like:

pub fn bad_function() -> RelayStr {
    RelayStr::new(&format!("Hello {}", "Jan"))
}

Copy link
Member

Choose a reason for hiding this comment

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

@jan-auer i feel like we should revisit this string pattern. I remember we incorrectly deduced what the allocation / borrow behavior once before.

Copy link
Member

Choose a reason for hiding this comment

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

@mitsuhiko Right, we had bugs prior to Relay in libsymbolic. It's worth noting that the Python layer always copies the string anyway.

@RaduW you're right, the name new doesn't carry the implications the function has. It is worth noting that RelayStr is only to be used in an unsafe FFI layer, so the exposure is pretty low. Let's use RelayStr::default for now, which will inherently not have any borrowing problems.

To continue a discussion from Slack: There is not much code, and it is easy to enumerate all places where we use RelayStr::new:

  • relay_data_category_name, which always returns static strings
  • RelayStr::default, which can be considered an implementation detail of empty strings in RelayStr
  • relay_str_from_cstr, which deals with pointers. It's also dead code
  • relay_validate_pii_config, which can also be changed to RelayStr::default
  • relay_convert_datascrubbing_config, which returns a static "{}".

To sum this up, we can address this much simpler by:

  • Changing the signature to new(s: &'static str) -> Self (this is now inherently safe)
  • Removing relay_str_from_cstr

@RaduW RaduW requested a review from jan-auer March 1, 2021 09:00
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.

Thanks, this is good to merge.

A side note on naming: One mentions "dynamic rule", the other "sampling". Given that both validate within the same structure, we could choose more similar names, such as validate_sampling_config and validate_sampling_condition.

let mut error_str: Option<String> = None;
for rule in config.rules {
if !rule.condition.supported() {
error_str = Some("unsupported sampling rule".to_string());
Copy link
Member

Choose a reason for hiding this comment

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

Can you just make this an early return? For example:

    match serde_json::from_str::<SamplingConfig>((*value).as_str()) {
        Ok(config) => {
            for rule in config.rules {
                if !rule.condition.supported() {
                    return RelayStr::new("unsupported sampling rule");
                }
            }
            
            RelayStr::default()
        }
        Err(e) => RelayStr::from_string(e.to_string()),
    }

Note that I'm using new here as per discussion in #941 (comment).

@RaduW RaduW merged commit 43f3381 into master Mar 3, 2021
@RaduW RaduW deleted the feat/cabi-dyn-sampling-validation-2 branch March 3, 2021 14:44
jan-auer added a commit that referenced this pull request Mar 9, 2021
* master:
  release: 0.8.4
  meta: Changelog
  feat(server): Add rule id to outcomes coming from event sampling (#943)
  feat(py): Expose dynamic rule condition validation (#941)
  feat(server): Log old events and sessions (#933)
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.

3 participants