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(trace): impl ShouldSample for Box<dyn ShouldSample> #2515

Closed
wants to merge 1 commit into from

Conversation

Oliboy50
Copy link

Fixes #
Design discussion issue (if applicable) #

Changes

Implement ShouldSample for Box<dyn ShouldSample> because:

  • users of this crate cannot do it themselves (because of the Rust orphan rule)
  • while working on a small wrapper around opentelemetry-sdk but tracer_provider_builder.with_sampler(...) currently only accepts a T: crate::trace::ShouldSample + 'static generic parameter... but I can't introduce generic types in my wrapper (for some reasons) and therefore I would like to use a dyn ShouldSample instead... and I currently can't do that

I'll update this PR to fix the "Merge requirement checklist" below, only if you are OK with it in the first place.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@Oliboy50 Oliboy50 requested a review from a team as a code owner January 15, 2025 16:46
Copy link

linux-foundation-easycla bot commented Jan 15, 2025

CLA Signed


The committers listed above are authorized under a signed CLA.

@utpilla
Copy link
Contributor

utpilla commented Jan 16, 2025

Changes

Implement ShouldSample for Box<dyn ShouldSample> because:

  • users of this crate cannot do it themselves (because of the Rust orphan rule)
  • while working on a small wrapper around opentelemetry-sdk but tracer_provider_builder.with_sampler(...) currently only accepts a T: crate::trace::ShouldSample + 'static generic parameter... but I can't introduce generic types in my wrapper (for some reasons) and therefore I would like to use a dyn ShouldSample instead... and I currently can't do that

If I understand the issue correctly, your wrapper version of with_sampler method can only take trait objects and not impl traits.
If that's the case, could you use a wrapper struct to get around this issue? Something like this:

fn with_sampler_custom(sampler: Box<dyn ShouldSample>) -> opentelemetry_sdk::trace::Builder {
    opentelemetry_sdk::trace::TracerProvider::builder().with_sampler(WrapperSampler(sampler))
}

#[derive(Debug, Clone)]
struct WrapperSampler(Box<dyn ShouldSample>);

impl ShouldSample for WrapperSampler {
    fn should_sample(
        &self,
        parent_context: Option<&Context>,
        trace_id: opentelemetry::trace::TraceId,
        name: &str,
        span_kind: &SpanKind,
        attributes: &[KeyValue],
        links: &[opentelemetry::trace::Link],
    ) -> opentelemetry::trace::SamplingResult {
        let sampler = self.0.as_ref();
        let result =
            sampler.should_sample(parent_context, trace_id, name, span_kind, attributes, links);
        result
    }
}

@Oliboy50
Copy link
Author

Oliboy50 commented Jan 16, 2025

@utpilla you are right, I don't know why I didn't think about using the NewType pattern to get around the orphan rule 🤦

I close my PR then, thank you for your help ❤️

@Oliboy50 Oliboy50 closed this Jan 16, 2025
@Oliboy50 Oliboy50 deleted the patch-1 branch January 16, 2025 08:59
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.

2 participants