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.

Sign up for GitHub

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(protocol): Support comparisons in rule conditions on strings #2721

Merged
merged 9 commits into from
Nov 16, 2023
Merged

feat(protocol): Support comparisons in rule conditions on strings #2721

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6aedc4f
feat(protocol): Support comparisons in rule conditions on strings
jan-auer Nov 14, 2023
70df1d4
ref: Introduce versioning for dynamic sampling
jan-auer Nov 14, 2023
9bff636
ref(dynamic-config): Bump metrics and sampling config versions
jan-auer Nov 14, 2023
761e366
fix(py): Test name
jan-auer Nov 14, 2023
636d382
fix: Broken tests
jan-auer Nov 14, 2023
b94bfe7
test(integration): Move sampling config
jan-auer Nov 15, 2023
a910d4b
meta: Changelog
jan-auer Nov 15, 2023
55912c3
ref: Address review feedback
jan-auer Nov 15, 2023
cb7db8e
fix(integration): Update tests with simpler assertion
jan-auer Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
- Scrub all DB Core Data spans differently. ([#2686](https://github.com/getsentry/relay/pull/2686))
- Support generic metrics extraction version 2. ([#2692](https://github.com/getsentry/relay/pull/2692))
- Emit error on continued project config fetch failures after a time interval. ([#2700](https://github.com/getsentry/relay/pull/2700))
- Support comparison operators (`>`, `>=`, `<`, `<=`) for strings in dynamic sampling and metric extraction rules. Previously, these comparisons were only possible on numbers. ([#2721](https://github.com/getsentry/relay/pull/2721))

## 23.10.1

Expand Down
37 changes: 32 additions & 5 deletions py/tests/test_processing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ def test_invalid_sampling_condition():
sentry_relay.validate_rule_condition(condition)


def test_validate_sampling_configuration():
def test_validate_legacy_sampling_configuration():
"""
Tests that a valid sampling rule configuration passes
"""
Expand All @@ -264,9 +264,9 @@ def test_validate_sampling_configuration():
"condition": {
"op": "custom",
"name": "event.legacy_browser",
"value":["ie10"]
"value": ["ie10"]
},
"id":1
"id": 1
},
{
"type": "trace",
Expand All @@ -277,10 +277,37 @@ def test_validate_sampling_configuration():
"condition": {
"op": "eq",
"name": "event.release",
"value":["1.1.*"],
"value": ["1.1.*"],
"options": {"ignoreCase": true}
},
"id": 2
}
]
}"""
# Should NOT throw
sentry_relay.validate_sampling_configuration(config)


def test_validate_sampling_configuration():
"""
Tests that a valid sampling rule configuration passes
"""
config = """{
"version": 2,
"rules": [
{
"type": "trace",
"samplingValue": {
"type": "sampleRate",
"value": 0.9
},
"condition": {
"op": "eq",
"name": "event.release",
"value": ["1.1.*"],
"options": {"ignoreCase": true}
},
"id":2
"id": 2
}
]
}"""
Expand Down
24 changes: 16 additions & 8 deletions relay-cabi/include/relay.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
/**
* Classifies the type of data that is being ingested.
*/
enum RelayDataCategory {
enum RelayDataCategory
{
/**
* Reserved and unused.
*/
Expand Down Expand Up @@ -106,7 +107,8 @@ typedef int8_t RelayDataCategory;
/**
* Controls the globbing behaviors.
*/
enum GlobFlags {
enum GlobFlags
{
/**
* When enabled `**` matches over path separators and `*` does not.
*/
Expand All @@ -129,7 +131,8 @@ typedef uint32_t GlobFlags;
/**
* Represents all possible error codes.
*/
enum RelayErrorCode {
enum RelayErrorCode
{
RELAY_ERROR_CODE_NO_ERROR = 0,
RELAY_ERROR_CODE_PANIC = 1,
RELAY_ERROR_CODE_UNKNOWN = 2,
Expand All @@ -154,7 +157,8 @@ typedef uint32_t RelayErrorCode;
* Values from <https://github.com/open-telemetry/opentelemetry-specification/blob/8fb6c14e4709e75a9aaa64b0dbbdf02a6067682a/specification/api-tracing.md#status>
* Mapping to HTTP from <https://github.com/open-telemetry/opentelemetry-specification/blob/8fb6c14e4709e75a9aaa64b0dbbdf02a6067682a/specification/data-http.md#status>
*/
enum RelaySpanStatus {
enum RelaySpanStatus
{
/**
* The operation completed successfully.
*
Expand Down Expand Up @@ -279,7 +283,8 @@ typedef struct RelayStoreNormalizer RelayStoreNormalizer;
* - When obtained as instance through return values, always free the string.
* - When obtained as pointer through field access, never free the string.
*/
typedef struct RelayStr {
typedef struct RelayStr
{
/**
* Pointer to the UTF-8 encoded string data.
*/
Expand All @@ -303,7 +308,8 @@ typedef struct RelayStr {
* - When obtained as instance through return values, always free the buffer.
* - When obtained as pointer through field access, never free the buffer.
*/
typedef struct RelayBuf {
typedef struct RelayBuf
{
/**
* Pointer to the raw data.
*/
Expand All @@ -321,7 +327,8 @@ typedef struct RelayBuf {
/**
* Represents a key pair from key generation.
*/
typedef struct RelayKeyPair {
typedef struct RelayKeyPair
{
/**
* The public key used for verifying Relay signatures.
*/
Expand All @@ -335,7 +342,8 @@ typedef struct RelayKeyPair {
/**
* A 16-byte UUID.
*/
typedef struct RelayUuid {
typedef struct RelayUuid
{
/**
* UUID bytes in network byte order (big endian).
*/
Expand Down
3 changes: 2 additions & 1 deletion relay-dynamic-config/src/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ pub struct CustomMeasurementConfig {
/// - 3:
/// - Emit a `usage` metric and use it for rate limiting.
/// - Delay metrics extraction for indexed transactions.
const TRANSACTION_EXTRACT_VERSION: u16 = 3;
/// - 4: Adds support for `RuleConfigs` with string comparisons.
const TRANSACTION_EXTRACT_VERSION: u16 = 4;

/// Deprecated. Defines whether URL transactions should be considered low cardinality.
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq)]
Expand Down
12 changes: 8 additions & 4 deletions relay-dynamic-config/src/project.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ pub struct ProjectConfig {
#[serde(skip_serializing_if = "Vec::is_empty")]
pub quotas: Vec<Quota>,
/// Configuration for sampling traces, if not present there will be no sampling.
#[serde(skip_serializing_if = "Option::is_none")]
pub dynamic_sampling: Option<SamplingConfig>,
#[serde(alias = "dynamicSampling", skip_serializing_if = "Option::is_none")]
pub sampling: Option<ErrorBoundary<SamplingConfig>>,
/// Configuration for measurements.
/// NOTE: do not access directly, use [`relay_event_normalization::DynamicMeasurementsConfig`].
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down Expand Up @@ -97,6 +97,10 @@ impl ProjectConfig {

metrics::convert_conditional_tagging(self);
defaults::add_span_metrics(self);

if let Some(ErrorBoundary::Ok(ref mut sampling_config)) = self.sampling {
sampling_config.normalize();
}
}
}

Expand All @@ -111,7 +115,7 @@ impl Default for ProjectConfig {
datascrubbing_settings: DataScrubbingConfig::default(),
event_retention: None,
quotas: Vec::new(),
dynamic_sampling: None,
sampling: None,
measurements: None,
breakdowns_v2: None,
performance_score: Default::default(),
Expand Down Expand Up @@ -150,7 +154,7 @@ pub struct LimitedProjectConfig {
#[serde(skip_serializing_if = "DataScrubbingConfig::is_disabled")]
pub datascrubbing_settings: DataScrubbingConfig,
#[serde(skip_serializing_if = "Option::is_none")]
pub dynamic_sampling: Option<SamplingConfig>,
pub sampling: Option<ErrorBoundary<SamplingConfig>>,
#[serde(skip_serializing_if = "SessionMetricsConfig::is_disabled")]
pub session_metrics: SessionMetricsConfig,
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down
17 changes: 10 additions & 7 deletions relay-protocol/src/condition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use relay_common::glob3::GlobPatterns;
use serde::{Deserialize, Serialize};
use serde_json::{Number, Value};
use serde_json::Value;

use crate::{Getter, Val};

Expand Down Expand Up @@ -106,12 +106,12 @@ macro_rules! impl_cmp_condition {
/// Path of the field that should match the value.
pub name: String,
/// The numeric value to check against.
pub value: Number,
pub value: Value,
Copy link
Member

Choose a reason for hiding this comment

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

Now since this is a value, should we also add array support like Eq has?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't wanna add this since the semantics of a comparison with an array are questionable. For eq, we defined it as "one of", but for comparisons this seemed weird to me. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I personally found it already surprising for Eq, usually this would be a In operator. So for me then I assumed the value can always be an array and it turns into something like value <OPERATOR> ANY(value).

I agree though this is weird but we already have a precedent for the array. Though I think we just leave it as is and implement it when we need it (we can bump versions now, yay)

}

impl $struct_name {
/// Creates a new condition that comparison condition.
pub fn new(field: impl Into<String>, value: impl Into<Number>) -> Self {
pub fn new(field: impl Into<String>, value: impl Into<Value>) -> Self {
Self {
name: field.into(),
value: value.into(),
Expand All @@ -136,6 +136,8 @@ macro_rules! impl_cmp_condition {
a $operator b
} else if let (Some(a), Some(b)) = (value.as_f64(), self.value.as_f64()) {
a $operator b
} else if let (Some(a), Some(b)) = (value.as_str(), self.value.as_str()) {
a $operator b
} else {
false
}
Expand Down Expand Up @@ -527,7 +529,7 @@ impl RuleCondition {
///
/// let condition = RuleCondition::gt("obj.length", 10);
/// ```
pub fn gt(field: impl Into<String>, value: impl Into<Number>) -> Self {
pub fn gt(field: impl Into<String>, value: impl Into<Value>) -> Self {
Self::Gt(GtCondition::new(field, value))
}

Expand All @@ -540,7 +542,7 @@ impl RuleCondition {
///
/// let condition = RuleCondition::gte("obj.length", 10);
/// ```
pub fn gte(field: impl Into<String>, value: impl Into<Number>) -> Self {
pub fn gte(field: impl Into<String>, value: impl Into<Value>) -> Self {
Self::Gte(GteCondition::new(field, value))
}

Expand All @@ -553,7 +555,7 @@ impl RuleCondition {
///
/// let condition = RuleCondition::lt("obj.length", 10);
/// ```
pub fn lt(field: impl Into<String>, value: impl Into<Number>) -> Self {
pub fn lt(field: impl Into<String>, value: impl Into<Value>) -> Self {
Self::Lt(LtCondition::new(field, value))
}

Expand All @@ -566,7 +568,7 @@ impl RuleCondition {
///
/// let condition = RuleCondition::lte("obj.length", 10);
/// ```
pub fn lte(field: impl Into<String>, value: impl Into<Number>) -> Self {
pub fn lte(field: impl Into<String>, value: impl Into<Value>) -> Self {
Self::Lte(LteCondition::new(field, value))
}

Expand Down Expand Up @@ -934,6 +936,7 @@ mod tests {
& RuleCondition::eq_ignore_case("trace.user.segment", "vip"),
),
("match no conditions", RuleCondition::all()),
("string cmp", RuleCondition::gt("trace.transaction", "t")),
];

let dsc = mock_dsc();
Expand Down
Loading
Loading