MetricReader allows invalid configurations

[marcalff]

The following schema:

        "MetricReader": {
            "type": ["object", "null"],
            "additionalProperties": false,
            "minProperties": 1,
            "maxProperties": 2,
            "properties": {
                "periodic": {
                    "$ref": "#/$defs/PeriodicMetricReader"
                },
                "pull": {
                    "$ref": "#/$defs/PullMetricReader"
                },
                "producers": {
                    "type": "array",
                    "items": {
                        "$ref": "#/$defs/MetricProducer"
                    }
                }
            }
        },

is causing semantic issues.

Previously, it contained only "periodic" and "pull" properties, with max properties = 1, meaning a metric reader is either periodic or pull.

When producers was added, max properties was changed to 2.
This allows "periodic" + "producers", or "pull" + "producers", which is the intent (as I understand).

This however also allows to have both "periodic" and "pull", which are no longer mutually exclusive.

This opens the schema validation to unintended configurations, pushing this downstream to the SDK implementation.

If this combination is not allowed, this must be clarified explicitly:

• for end users, to know what to expect
• for SDK implementors, to test explicitly this case, so it can be handled correctly.

Found while implementing config.yaml for C++:

• [CONFIGURATION] Implement declarative configuration (config.yaml) opentelemetry-cpp#2518

[marcalff]

A possible fix is to move the "producers" property in each "periodic" and "pull" node.

[marcalff]

Relevant links:

• Add metric producers to meter_provider configuration #90
• Remove unnecessary MetricProducer in pull metric reader #93
• https://github.com/open-telemetry/opentelemetry-configuration/pull/90/files#r1608621210
• https://github.com/open-telemetry/opentelemetry-configuration/pull/90/files#r1608854357

[marcalff]

According to the schema, a metric reader can also have only "producers", as this satisfies min properties = 1.

This is invalid, I think this part of the schema should be revisited.

[jack-berg]

We could add documentation to type_descriptions.yaml that states that including both periodic and pull is invalid. We could also use oneOf or if / else to describe the that the properties are mutually exclusive. I have vague memory of jsonschema java tooling having bad / no support for anyof, oneof, but can't seem to find any old issues / PRs where its discussed. Even if jsonschema tooling struggles to enforce it, it could still be good to express in the schema.

[brettmc]

+1 that this part of the schema is difficult to implement as-is. I am quite stuck when trying to devise a working implementation for PHP.

A possible fix is to move the "producers" property in each "periodic" and "pull" node.

I haven't tested it out, but I do feel that this would be a lot easier to implement...