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

fix(sampling): Forward compatibility for config #1641

Merged
merged 4 commits into from
Nov 29, 2022
Merged

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Nov 28, 2022

Add missing variants to enums in sampling config for forward compatibility.

#skip-changelog

@jjbayer jjbayer marked this pull request as ready for review November 28, 2022 16:37
@jjbayer jjbayer requested a review from a team November 28, 2022 16:37
Comment on lines +75 to +79
SamplingMode::Unsupported => {
if processing_enabled {
relay_log::error!("Found unsupported sampling mode even as processing Relay, keep");
}
return Err(SamplingResult::Keep);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I like this, but I don't have better suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

We could alternatively log an error for during project state deserialization (since that's where the error really happens) and fail silently here instead.

This behavior is fine since it's the same as if sampling were not implemented. That matches behavior in other cases, where Relay ignores all configuration it doesn't know about.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could alternatively log an error for during project state deserialization (since that's where the error really happens)

I'd rather log the error during deserialization.

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 tried to be consistent with check_unsupported_rules here, but maybe it's a bad pattern anyway.

fn check_unsupported_rules(
processing_enabled: bool,
sampling_config: &SamplingConfig,
) -> Result<(), SamplingResult> {
// when we have unsupported rules disable sampling for non processing relays
if sampling_config.has_unsupported_rules() {
if !processing_enabled {
return Err(SamplingResult::Keep);
} else {
relay_log::error!("found unsupported rules even as processing relay");
}
}
Ok(())
}

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed offline, going to merge as-is.

@@ -372,7 +376,7 @@ pub struct SamplingRule {

impl SamplingRule {
fn supported(&self) -> bool {
self.condition.supported()
self.condition.supported() && self.ty != RuleType::Unsupported
Copy link
Member

Choose a reason for hiding this comment

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

nit: Introduce a supported method on rules types, too?

Comment on lines +75 to +79
SamplingMode::Unsupported => {
if processing_enabled {
relay_log::error!("Found unsupported sampling mode even as processing Relay, keep");
}
return Err(SamplingResult::Keep);
Copy link
Member

Choose a reason for hiding this comment

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

We could alternatively log an error for during project state deserialization (since that's where the error really happens) and fail silently here instead.

This behavior is fine since it's the same as if sampling were not implemented. That matches behavior in other cases, where Relay ignores all configuration it doesn't know about.

@jjbayer jjbayer enabled auto-merge (squash) November 29, 2022 13:23
@jjbayer jjbayer merged commit 72a8a61 into master Nov 29, 2022
@jjbayer jjbayer deleted the fix/fw-sampling-config branch November 29, 2022 14:03
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.

4 participants