open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.64k stars 871 forks source link

No limits for log record body #4006

Open pellared opened 2 months ago

pellared commented 2 months ago

What are you trying to achieve?

According to the docs there are only limits for the log attributes. See: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#logrecord-limits

LogRecord attributes MUST adhere to the common rules of attribute limits.

From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/README.md#attribute-limits:

If there are no limits placed on attributes, they can quickly exhaust available memory, resulting in crashes that are difficult to recover from safely.

The same problem is applicable for log record body value.

Is it intentional or an oversight? Should the limits be also applied for body value (even though that the whole naming is around attributes)? Should we have different limits for log record value?

pellared commented 2 months ago

PTAL @open-telemetry/specs-logs-approvers

djaglowski commented 2 months ago

An important scaling factor for attributes is that they are commonly used by backends for indexing or other optimizations. Do we expect the body to be treated similarly by backends? If not, we probably don't need to apply the same strict limits, though other limits may be necessary.

Related, how would these limits apply to a non-structured body?

jack-berg commented 2 months ago

We might not want the limits by default, but I think its reasonable to have them available. Currently, its not possible to configure any limits for the body, and its cumbersome to do in the collector (see this slack convo - it previously wasn't possible).

Related, how would these limits apply to a non-structured body?

The key question is how do attribute limits work on an AnyValue type. Note this will be relevant for events as well.

The common attribute limits provide configuration for the max allowed attribute count, and the maximum attribute length limit.

AnyValue is a recursive data structure, and the common attribute limits were designed for a flat list of key value pairs. Here's how we might apply the rules to AnyValue:

Note that the current rules don't limit the number of entries in an array, which is odd. I've carried that rule forward into the proposal above, but perhaps there should be a third property which limits the number of array entries.

pellared commented 2 months ago

@jack-berg, I do not think that it would be good to use AttributeValueLengthLimit and AttributeCountLimit for Body. Reasons:

  1. "we probably don't need to apply the same strict limits, though other limits may be necessary." by @djaglowski
  2. While Body uses the same "structure" as Attributes they are not semantically equivalent therefore I do not think we should use a single configuration to control both of them.

For now, (as a baby-step) I simply propose to clarify in the specification that the attribute limits do not apply on the Body.

Side note: SeverityText has also no limits.