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

custom samplers #102

Open
brettmc opened this issue Jul 10, 2024 · 5 comments
Open

custom samplers #102

brettmc opened this issue Jul 10, 2024 · 5 comments
Labels
blocked:spec The issue is blocked on having spec language on the topic.

Comments

@brettmc
Copy link
Contributor

brettmc commented Jul 10, 2024

The file-configuration schema allows for extra properties in the sampler, allowing me to define a sampler other than the ones defined in spec.

I'm in the process of implementing a rule-based sampler, based on Java's implementation. I foresee a collision in the future where Java SIG will also want to add and configure a rule-based sampler, and it probably won't have the same schema.

How can we support the configuration of custom/non-spec-defined samplers such that we can coexist with future samplers?

For reference, an example of our custom rule-based sampler configuration:

tracer_provider:
  sampler:
    rule_based:
      rule_sets:
        - rules:
            - attribute: {key: is.important, pattern: ~false~ }
          delegate:
            always_off: {}
        - rules:
            - link: { sampled: true }
          delegate:
            always_on: {}
        - rules:
            - span_name: { pattern: ~foobar~ }
          delegate:
            always_off: {}
        - rules:
            - span_kind: { kind: SERVER }
            - attribute: { key: url.path, pattern: ~^/health$~ }
          delegate:
            always_off: {}
      fallback:
        always_on: {}

The sampler runs through rules until it finds a match, and then delegates the decision to the configured sampler for that rule. If no match, fall back to a different sampler.

@marcalff
Copy link
Member

In my understanding, having implemented this for opentelemetry-cpp:

For a native sampler (always_on), the schema is fixed, and the list of samplers known comes from the schema.

For a custom sampler (rule_based), there is no schema enforced.
The list of custom samplers comes from what component is registered:

Is your concern that the name "rule_based" may collide later, if a native sampler of that name is added ?

I think the issue will not be with the schema, it will happen sooner, when the registration fails, because a sampler of that name already exists.

The type and name comprise a unique key. Register MUST return an error if it is called multiple times with the same type and name combination.

The spec is unclear for built-in components, since it is unclear if native (built-in) components are "pre registered" or not, but I would expect registration colliding with native names to fail.

@marcalff
Copy link
Member

So the question becomes, how to share a namespace between native and custom components.

@brettmc
Copy link
Contributor Author

brettmc commented Jul 15, 2024

Is your concern that the name "rule_based" may collide later, if a native sampler of that name is added ?

Not really, I think in that scenario we would manage the transition from the custom to native..

My main concern is in a setup like the otel demo app, if we wanted to use one configuration file to configure multiple services. rule_based might mean something different to Java, or might not be implemented at all.

I think that right now, a single file-based configuration could be applied to any SIG and would work. But the use of a custom sampler in one or more SIGs/services would mean we might need multiple config files. I'm not sure whether it was ever an intention that one config file could be used to configure all SIGs, but I assumed it was.

edit: I hadn't noticed the example about merging multiple configurations, and that might actually resolve my concerns above...

@jack-berg
Copy link
Member

How can we support the configuration of custom/non-spec-defined samplers such that we can coexist with future samplers?

Short of informal coordination across language sigs, I don't think we can avoid collisions until someone successfully drives defining a rule based sampler at the spec level. IMO, its a obvious missing piece. I just don't have capacity to own it.

@brettmc
Copy link
Contributor Author

brettmc commented Jul 17, 2024

until someone successfully drives defining a rule based sampler at the spec level...

I submitted an initial PR for a rule-based sampler, but it looks like OTEP-250 encompasses this. If that's going to get up, presumably a spec change will follow which will provide for a rule-based sampler.

I think the safest approach for me is to give my sampler a key like contrib_rule_based that won't conflict with a future rule-based sampler.

@jack-berg jack-berg added the blocked:spec The issue is blocked on having spec language on the topic. label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:spec The issue is blocked on having spec language on the topic.
Projects
Status: Not blocking stability
Development

No branches or pull requests

3 participants