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

No limits for log record body #4006

Open
pellared opened this issue Apr 23, 2024 · 6 comments
Open

No limits for log record body #4006

pellared opened this issue Apr 23, 2024 · 6 comments
Labels
spec:logs Related to the specification/logs directory triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted

Comments

@pellared
Copy link
Member

pellared commented Apr 23, 2024

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 pellared added the spec:logs Related to the specification/logs directory label Apr 23, 2024
@pellared
Copy link
Member Author

PTAL @open-telemetry/specs-logs-approvers

@djaglowski
Copy link
Member

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
Copy link
Member

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.

@austinlparker austinlparker added the triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted label Apr 23, 2024
@pellared
Copy link
Member Author

pellared commented Apr 24, 2024

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

@svrnm
Copy link
Member

svrnm commented Sep 30, 2024

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

@svrnm svrnm added triage:deciding:needs-info Not enough information. Left open to provide the author with time to add more details and removed triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted labels Sep 30, 2024
@pellared
Copy link
Member Author

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

@trask trask added triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted and removed triage:deciding:needs-info Not enough information. Left open to provide the author with time to add more details labels Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:logs Related to the specification/logs directory triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted
Projects
None yet
Development

No branches or pull requests

6 participants