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

Define schema for simple propagator #75

Closed
marcalff opened this issue Feb 22, 2024 · 3 comments
Closed

Define schema for simple propagator #75

marcalff opened this issue Feb 22, 2024 · 3 comments

Comments

@marcalff
Copy link
Member

marcalff commented Feb 22, 2024

Currently, propagator.json reads as:

{
    "$id": "https://opentelemetry.io/otelconfig/propagator.json",
    "$schema": "https://json-schema.org/draft/2020-12/schema",
    "title": "Propagator",
    "type": "object",
    "minProperties": 1,
    "maxProperties": 1,
    "additionalProperties": true,
    "properties": {
        "composite": {
            "type": "array",
            "items": {
                "type": "string"
            }
        }
    }
}

This defines how a composite proparator is represented, but leaves the definition for the simple propagator open to any interpretation, with additional properties.

All the following can be considered valid [1] per the schema:

propagator:
  tracecontext
propagator:
  this_also:
    is_valid: yes
    is_intented: probably_not
    use: tracecontext

Please add another property for simple, with a type string, and do not allow additionalProperties.

The intended representation should be:

propagator:
  simple: tracecontext

Not well versed in json-schema, but I would expect something like:

{
    "$id": "https://opentelemetry.io/otelconfig/propagator.json",
    "$schema": "https://json-schema.org/draft/2020-12/schema",
    "title": "Propagator",
    "type": "object",
    "minProperties": 1,
    "maxProperties": 1,
    "properties": {
        "composite": {
            "type": "array",
            "items": {
                "type": "string"
            }
        },
        "simple": {
            "type": "string"
        }
    }
}

[1] From casually reading the schema and writing yaml, I did not try a validator.

@jack-berg
Copy link
Member

@marcalff what is the advantage of specifying simple propagator? Isn't it the same as composite propagator with a single entry?

@marcalff
Copy link
Member Author

@marcalff what is the advantage of specifying simple propagator? Isn't it the same as composite propagator with a single entry?

A "composite" propagator with only one entry will work too.

If we want to model it this way:

  • "additionalProperties": true, is misleading, there will never be additional properties
  • consider renaming composite

The spec defines a composite as "multiple propagators", I would assume multiple means N > 1, not N >= 1.

Basically, when reading:

    "minProperties": 1,
    "maxProperties": 1,
    "additionalProperties": true,

it means there is either one property named composite, or another unspecified property, hence the issue report.

@marcalff
Copy link
Member Author

marcalff commented Dec 3, 2024

Resolved by #141

The following was removed:

"minProperties": 1,
"maxProperties": 1,
"additionalProperties": true,

so that there can no longer by arbitrary nodes used as an extension for a simple propagator.

For a simple propagator, the modeling should use a composite propagator, with an array of one element.

Closing as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants