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

Revisit 1024 char limit on label values #14721

Closed
carsonip opened this issue Nov 25, 2024 · 13 comments
Closed

Revisit 1024 char limit on label values #14721

carsonip opened this issue Nov 25, 2024 · 13 comments

Comments

@carsonip
Copy link
Member

carsonip commented Nov 25, 2024

There is a 1024 character (as in unicode characters) limit on string label values, on otlp (see code) and intakev2, because we used to depend on Beats & Fleet to set up mappings which would both set ignore_above: 1024 on keyword fields. As apm-server has migrated to ES apm-data plugin, revisit this limitation on label value.

#14716 is a related issue to handle an inconsistency in enforcing this limit.

@carsonip carsonip changed the title Revisit 1024 char limit for label values Revisit 1024 char limit on label values Nov 25, 2024
@inge4pres
Copy link
Contributor

AFAIR OpenTelemetry is also enforcing some limits on the Attributes key/value length?
Is it true? If yes, how do we discern the APM format, if we'd need to?

@carsonip
Copy link
Member Author

AFAIR OpenTelemetry is also enforcing some limits on the Attributes key/value length?

There is truncation for OTel attributes but default truncation length is unlimited by default: https://opentelemetry.io/docs/specs/otel/common/#attribute-limits

@kruskall
Copy link
Member

A few things discovered while working on this:

  • labels are not the only fields limited to 1024 as pointed out above
  • this is not just otlp. Intake also has fields limited to 1024
  • while it's true that apm-data is no longer using ignore_above, the new otel-data plugin is using ignore_above: 1024 in quite a few places

Given the above, I wonder if we should consistently remove the 1024 limit everywhere. WDYT ?

@mlunadia
Copy link

mlunadia commented Jan 7, 2025

I don't have any data on the impact of this that can help me form a useful opinion.

  • Is this just changing a default which would be reversible?
  • Do we have any customers who have raised an issue about data being truncated with the current limits?

@kruskall
Copy link
Member

kruskall commented Jan 9, 2025

Is this just changing a default which would be reversible?

it could be reversible but the limit is hardcoded and it would require a new version. Another option could be a user manually configuring a pipeline to limit the field length.

@raultorrecilla
Copy link

moving this to it 106

@simitt
Copy link
Contributor

simitt commented Jan 13, 2025

The APM spec defines:

The maximum length of metadata, transaction, span, et al fields are determined
by the APM Server Events Intake API schema.
Except for special cases, fields are typically limited to 1024 unicode characters.
Unless listed below as knowm "long fields", agents SHOULD truncate filed values to 1024 characters, as specified below.

Therefore only removing the 1024 field limits might not make an actual change for users leveraging Elastic APM agents (Though it is a SHOULD and not a MUST spec, so not every agent might have it implemented).
I am not aware of specific recent problems reported with this, but the 1024 field limits have not been revisited for a long time, so might be good to take a look. Making the config user adjustable to become closer to Otel spec sounds reasonable. If the limits get changed, ensure to let the agent dev team know so they can consider updates in the Elastic APM agents.

If we move forward with custom config, this can be done in two ways

  • breaking change: changing the default, e.g. from 1024 to unlimited -> the change needs to go into 9.0.
  • non-breaking: make it configurable but keep 1024 as default

The solution chosen defines the urgency. @mlunadia any preference from product perspective?

@simitt
Copy link
Contributor

simitt commented Jan 20, 2025

@mlunadia friendly ping for input on this.

@simitt
Copy link
Contributor

simitt commented Jan 24, 2025

@felixbarny do you have any insights on why the new otel-data plugin is also defining 1024 char limit on various fields (see #14721 (comment)).
When moving forward with this, we should at least ensure that we are not introducing a behavior in apm-server that is intended to be aligned with otel standard, but then is not aligned with what is currently built for the otel native storage format.

@felixbarny
Copy link
Member

Both apm-data and otel-data effectively still use ignore_above: 1024 for keyword fields as they both import ecs@mappings that maps all strings to keywords with ignore_above: 1024. This is to protect against hitting a hard limit of 32kb in Lucene. Only fields that go beyond this limit are ignored, this doesn't lead rejected documents - even if the failure store is disabled.

Having said all of that, I'm not sure if it makes sense to make changes to APM Server and especially all classic agents as we're on a path towards succeeding APM components with OTel/EDOT. Therefore, we should only spend energy on APM components if this is fixing a bug or if it otherwise lowers our maintenance burden.

@simitt
Copy link
Contributor

simitt commented Jan 28, 2025

In alignment with @felixbarny 's point of otel-data and apm-data still pulling in ECS mappings with the ignore_above: 1024 restrictions, I propose to not move forward with removing the 1024 char limit for 9.0. It would lead to a mixed setup, as it's unlikely that we could change the ECS mappings ad-hoc. Afaik we haven't heard of customer issues with this.
Elasticsearch by default sets a limit of 256 for keywords (ref), which is more restrictive than what is set for apm/otel.

@kruskall
Copy link
Member

Elasticsearch by default sets a limit of 256 for keywords (ref), which is more restrictive than what is set for apm/otel.

TIL!

fwiw given the above I'm also ok with keeping the 1024 limit (I've changed the apm-data PR to draft. We can close it along with this issue).

@simitt
Copy link
Contributor

simitt commented Jan 28, 2025

We just synced on this and decided to not move forward with this change for now for the above discussed reasons.

@simitt simitt closed this as completed Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants