opentelemetry-specification icon indicating copy to clipboard operation
opentelemetry-specification copied to clipboard

No limits for log record `Body`, `SeverityText`, and map/array/bytes `Attributes`

Open pellared opened this issue 1 year ago • 6 comments
trafficstars

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 avatar Apr 23 '24 06:04 pellared

PTAL @open-telemetry/specs-logs-approvers

pellared avatar Apr 23 '24 06:04 pellared

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?

djaglowski avatar Apr 23 '24 14:04 djaglowski

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:

  • AnyValue is oneof: string_value, bool_value, int_value, double_value, array_value, kv_list_value, bytes_value
    • If bool_value, int_value, double_value, ignore attribute limits
    • If string_value, bytes_value, truncate the value to the AttributeValueLengthLimit
    • If kv_list_value, limit the entires to AttributeCountLimit, apply limits recursively to each AnyValue value
    • If array_value, apply limits recursively to each AnyValue value

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.

jack-berg avatar Apr 23 '24 14:04 jack-berg

@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.

pellared avatar Apr 24 '24 06:04 pellared

@pellared @jack-berg please take a look, is this still needed? how can we move this forward?

svrnm avatar Sep 30 '24 09:09 svrnm

Hard for me to evaluate as Logs are still Beta in OTel Go and personally, I have no feedback on how much this feature is needed. I think we can still leave a "community-feedback".

pellared avatar Sep 30 '24 10:09 pellared