ref(rules): Move rules DSL to relay-protocol #2608
Conversation
relay-cabi/src/processing.rs
Outdated
| use relay_protocol::RuleCondition; | ||
| use relay_protocol::{Annotated, Remark}; |
There was a problem hiding this comment.
Please excuse the late review. See the comments below for a follow-up PR.
Eventually, we can look for an even better place for these types; relay-protocol is not the intended crate for this, as it is meant to contain foundational types for building our data schemas, which rule conditions are not part of. That said, I also haven't found the right place yet.
| mod value; | ||
|
|
||
| pub use self::annotated::*; | ||
| pub use self::condition::*; |
There was a problem hiding this comment.
This module contains a lot of types that usually do not need to be imported. By glob-exporting them, we're littering the global crate namespace as can be seen in the docs now.
When you compare the imports with how relay-sampling was structured, there was an explicit re-export of the RuleCondition type, as this is the most commonly used one, but all the other types were left in a pub mod condition instead. I'd prefer if we do this here, too.
That is independent of whether other modules are glob-exported or not.
| pub use self::impls::*; | ||
| pub use self::macros::*; | ||
| pub use self::meta::*; | ||
| pub use self::size::*; | ||
| pub use self::traits::*; | ||
| pub use self::utils::*; |
There was a problem hiding this comment.
Definitely do not glob-export internal utils to a public namespace. This file contains the function is_default, which shouldn't be part of the public API of relay-protocol. Instead, we should remove that module and just declare the function locally where needed.
There was a problem hiding this comment.
I thought about this, you are right that it's not good. Definitely needs to be re-worked.
| use crate::config::{ | ||
| DecayingFunction, RuleId, RuleType, SamplingRule, SamplingValue, TimeRange, | ||
| }; | ||
| use crate::dsc::TraceUserContext; | ||
| use crate::evaluation::MatchedRuleIds; | ||
| use crate::DynamicSamplingContext; | ||
| use relay_protocol::RuleCondition; |
There was a problem hiding this comment.
Please move this import into the import group above.
Follow up to #2608: * Remove `utils` crate from `relay-protocol`. * Only export `RuleCondition` from the `condition` module`. I was not able to remove the `relay-sampling` dependency anywhere yet, because `relay-cabi` and `relay-dynamic-config` import `SamplingConfig`. But we will be able to change the dependency in #2595.
This PR moves the
conditionmodule fromrelay-samplingtorelay-protocol. In order to perform the refactor, an adaptation of tests had to be done, in which dependencies fromrelay-samplingwhere removed by adding mock structs that implementGetter.