opentelemetry-sdk: Implement meter configurator

[herin049]

Description

Follow up to #4861 to implement part of the Metrics SDK spec to support meter configuration https://opentelemetry.io/docs/specs/otel/metrics/sdk/#meterconfigurator

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

tox -e py314-test-opentelemetry-sdk

Does This PR Require a Contrib Repo Change?

• Yes. - Link to PR:
• No.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[xrmx]

I see you implemented this like my first version of the TracerConfigurator storing and updating MeterConfigs and not the latest that is building that on the fly in _is_enabled.

[herin049]

Yes, I wanted to discuss this with you. Based on this section from the spec, it seems like storing the configuration object per Meter and only updating when the configurator changes is the most natural

This function is called when a Meter is first created, and for each outstanding Meter when a MeterProvider’s MeterConfigurator is updated (if updating is supported). Therefore, it is important that it returns quickly.

However, the primary reason I deviated from your approach is because the performance implications are much greater with metrics, given that the frequency of calls to metric instruments can in many cases be an order of magnitude greater than the number of calls to create a Span via a Tracer.

Is there a reason you decided to alter your approach to the one in the current implementation for the tracer configurator? The spec doesn't explicitly mention it, but I would expect that each configurator should be idempotent.

My reasoning is that this approach will in the end be much more efficient/maintainable, you only pay the update cost on initialization and when you update the configurator (which should be extremely rare). Also, I presume the configurator functionality will be extended at some point, so it is likely that the number of places each configuration is referenced will continue to grow, which could potentially become an issue with an "on-the-fly" method.

[xrmx]

It was discussed here #4861 (comment) with @aabmass . I think the difference here is also that you already have a cache of the meters in the MeterProvider and that for some reasons I don't recall (maybe wondering about Tracers going away) implemented that using a WeakSet instead of a plain dict as done here.
We're still in time to correct that for traces since everything is considered to be internal.

[herin049]

Thanks for the clarification, I'll bring this up in the SIG tomorrow.

[xrmx]

Here's a PR hopefully making the implementation similar #5007

[xrmx]

We should probably use meter_configurator directly since self._meter_configurator is outside the lock and on two concurrently calls we may end up applying a different tracer_configurator. Or maybe concurrency is not an issue here

[xrmx]

Also I think we may need to catch whatever exceptions this may raise to avoid breaking the sdk in case the user provides a buggy configurator

[herin049]

Good catch with the locking, I think we actually probably want to just update the meter configurator in the lock since it is also used in get_meter

[xrmx]

Unrelated to this PR but I'm thinking on making the rules updatable , WDYT?

class _RuleBasedTracerConfigurator:
    def __init__(
        self,
        *,
        rules: _TracerConfiguratorRulesT,
        default_config: _TracerConfig,
    ):
        self._rules = rules
        self._default_config = default_config

    def __call__(self, tracer_scope: InstrumentationScope) -> _TracerConfig:
        for predicate, tracer_config in list(self._rules):
            if predicate(tracer_scope):
                return tracer_config

        # if no rule matched return the default config
        return self._default_config

    def update_rules(self, rules: _TracerConfiguratorRulesT):
        self._rules = rules

[herin049]

Sure, let me add this into the PR

[herin049]

@xrmx Actually, we can probably make this generic to reduce code duplication. I can make this change in a later PR for the logs changes. Lmk what you think.

from typing import TypeVar, Generic, Callable

_ConfigT = TypeVar("_ConfigT")
_RulesT = list[tuple[Callable[[InstrumentationScope], bool], _ConfigT]]


class _RuleBasedConfigurator(Generic[_ConfigT]):
    def __init__(
        self,
        *,
        rules: _RulesT[_ConfigT],
        default_config: _ConfigT,
    ):
        self._rules = rules
        self._default_config = default_config

    def __call__(self, scope: InstrumentationScope) -> _ConfigT:
        for predicate, config in self._rules:
            if predicate(scope):
                return config
        return self._default_config

    def update_rules(self, rules: _RulesT[_ConfigT]) -> None:
        self._rules = rules

[xrmx]

What do you think on making these comparable, I've added the same to _TracerConfig, it's handy for tests and to compare rules:

    def __eq__(self, other: "_MeterConfig"):
        return self.is_enabled == other.is_enabled

[herin049]

Don't dataclasses generate an __eq__ method by default?