-
Notifications
You must be signed in to change notification settings - Fork 104
ref(processor): Move more functions #5322
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
Conversation
| project_id: ProjectId, | ||
| ctx: processing::Context<'_>, | ||
| geo_lookup: &GeoIpLookup, | ||
| reservoir_counters: &ReservoirEvaluator<'_>, |
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.
This argument was passed into the function but was disabled by supports_reservoir_sampling on the Sampled trait.
| ) | ||
| .await; | ||
| // If no metrics could be extracted, do not sample anything. | ||
| let should_sample = matches!(&ctx.project_info.config().metric_extraction, ErrorBoundary::Ok(c) if c.is_supported()); |
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.
This check was previously evaluated in supports_sampling in the Sampling trait.
| // Also, we require transaction metrics to be enabled before sampling. | ||
| let run_dynamic_sampling = (matches!(filter_run, FiltersStatus::Ok) | ||
| || self.inner.config.processing_enabled()) | ||
| && matches!(&ctx.project_info.config.transaction_metrics, Some(ErrorBoundary::Ok(c)) if c.is_enabled()); |
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.
This check was previously defined in Sampled::supports_sampling.
Like in #5309, refactor processing helpers to not depend on envelopes / the processor service itself, and move them to
processing.ref: INGEST-610