-
Notifications
You must be signed in to change notification settings - Fork 227
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
feat: Add "longFieldMaxLength" config var #2193
Conversation
Before this the 10000 length truncation of the "query" field for database spans was hardcoded. Fixes: #1921
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
elastic/apm#488 is a discussion for a cross-agent config option name for this var. Let's hold until that discussion plays out. |
I've started elastic/apm#493 for the spec addition for this field name: |
^^ Integration Tests failure is the current elastic/apm-integration-testing#1188 flake |
Here is a small test script to demonstrate const apm = require('./').start({
longFieldMaxLength: 100000
})
const { Client } = require('@elastic/elasticsearch')
var client = new Client({
node: 'http://localhost:9200',
auth: { username: 'admin', password: 'changeme' }
})
const t1 = apm.startTransaction('t1')
// Build up a search body >10k in size.
aggs = {}
for (var i = 0; i < 1000; i++) {
aggs[`aggs${i}`] = { "terms": { "field": "author" } }
}
client.search(
{
body: {
size: 1,
aggregations: aggs
}
},
function (err, result) {
t1.end()
}
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks it does the thing the way the agent wants the thing done. Will approve once the package.json
is updated to reflect what we want to ship.
- Truncate on number of JS chars (i.e. a count of UCS-2 chars), not bytes. This is faster, sufficient for ES storage concerns for large fields, and matches the json-schema spec for fields with a "maxLength". - Remove truncateQueriesAt, add truncateLongFieldsAt which covers that and the other fields specified for long_field_max_length. - Deprecate truncateErrorMessagesAt. If unspecified, the truncateLongFieldsAt value will be used. BREAKING CHANGE. Refs: elastic/apm-agent-nodejs#2193
…MaxLength` (elastic#2193) This config option will truncate some transaction, span, and error fields that can be longer at the configured number of JavaScript characters per https://github.com/elastic/apm/blob/master/specs/agents/field-limits.md#long_field_max_length-configuration Notably truncation is now based on a count of JavaScript *characters* rather than in UTF-8 encoded bytes. This also deprecates the errorMessageMaxLength config option. It now defaults to the longFieldMaxLength value if not specified. Otherwise errorMessageMaxLength is still supported. Fixes: elastic#1921
…MaxLength` (#2193) This config option will truncate some transaction, span, and error fields that can be longer at the configured number of JavaScript characters per https://github.com/elastic/apm/blob/master/specs/agents/field-limits.md#long_field_max_length-configuration Notably truncation is now based on a count of JavaScript *characters* rather than in UTF-8 encoded bytes. This also deprecates the errorMessageMaxLength config option. It now defaults to the longFieldMaxLength value if not specified. Otherwise errorMessageMaxLength is still supported. Fixes: #1921
elastic/apm#493 specifies a
long_field_max_length
config option we can use forspan.context.db.statement
. Currently this is hardcoded to 10000... bytes.long_field_max_length
. See the http-client PR for this part of the change.errorMessageMaxLength
config option. It now defaults to thelongFieldMaxLength
value if not specified. OtherwiseerrorMessageMaxLength
is still supported. (We should drop it in the "next major".)long_field_max_length
spec. This does mean that some existing usage out there might get truncation of some of these fields where they didn't before. That's debatable whether that constitutes a "breaking change". So far this PR takes the point of view that this is not a breaking change that would require either a separate config var (which means not conforming with the APM spec link above) or only applying truncation to these fields in a new major version later. Objections?Fixes: #1921
Checklist