open-telemetry / opentelemetry-configuration

JSON Schema definitions for OpenTelemetry file configuration
Apache License 2.0
29 stars 15 forks source link

[EXAMPLES] Periodic exporter interval default value is inconsistent. #108

Open marcalff opened 1 month ago

marcalff commented 1 month ago

The examples reads as:

    # Configure a periodic metric reader.
    - periodic:
        # Configure delay interval (in milliseconds) between start of two consecutive exports.
        #
        # Environment variable: OTEL_METRIC_EXPORT_INTERVAL
        interval: 5000
        # Configure maximum allowed time (in milliseconds) to export data.
        #
        # Environment variable: OTEL_METRIC_EXPORT_TIMEOUT
        timeout: 30000

Per the spec:

the default value for interval is 60000.

The problem here is not so much the values used (examples are free to use anything), but the fact that this setup is invalid, at least for opentelemetry-cpp:

Since every other numerical value provided in examples seem to match defaults, please consider to use the default (60000) for interval, to avoid confusion.

Currently:

[malff@malff-desktop examples]$ grep "interval:" *
anchors.yaml:        interval: 5000
kitchen-sink.yaml:        interval: 5000
sdk-config.yaml:        interval: 60000
sdk-migration-config.yaml:        interval: ${OTEL_METRIC_EXPORT_INTERVAL:-60000}

Files anchors.yaml and kitchen-sink.yaml use 5000 instead.

jack-berg commented 1 month ago

Good find.

The problem here is not so much the values used (examples are free to use anything)

While examples are free to use anything, I think its helpful for us to have consistent patterns we follow. The sdk-config.yaml and sdk-migration-config.yaml target users, and so their default values do actually matter and should align with defaults. The kitchen-sink.yaml is contrived and can use any values, but its still probably helpful to use the defaults. One problem with the defaults is that they're not always defined. For example, the default value for .attribute_limits.attribute_value_length_limit is undefined. However, we do want to demonstrate setting some value for this in the example to verify we have the correct type in the schema. Maybe we can adopt a strategy of using the default when available, and when unavailable, using some dummy value with a callout that its not the default.