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

Add long_field_max_length configuration option #493

Merged
merged 5 commits into from
Aug 27, 2021

Conversation

trentm
Copy link
Member

@trentm trentm commented Aug 23, 2021

This is a follow up to the discussion issue for a common config var for some of the longer fields that (a) aren't limited in length by the APM server intake API schema and (b) have some current agent-specific issues open to be made configurable.

Rendered here: https://github.com/trentm/apm/blob/long_field_max_length/specs/agents/field-limits.md

Some reviewer notes:

  • The user issues that led to this config option were for {transaction,error}.context.request.body and span.context.db.statement only. I added some other potential candidate fields for this config var {transaction,span,error}.context.message.body, error.exception.message, error.log.message. I have no strong preference on whether to include those other fields now, later or never. There are here for discussion. Thoughts? (The Node.js APM agent already has the errorMessageMaxLength config var for the latter two.)
  • I'm ambivalent on "Central config: true" for this. It is certainly feasible, but are there reasons we'd consider avoiding adding rarely used config vars to the central config UI?

@apmmachine
Copy link

apmmachine commented Aug 23, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-25T15:30:36.880+0000

  • Duration: 5 min 48 sec

  • Commit: 2deecbe

Trends 🧪

Image of Build Times

@trentm trentm marked this pull request as ready for review August 24, 2021 17:37
@trentm trentm requested review from a team as code owners August 24, 2021 17:37
@trentm
Copy link
Member Author

trentm commented Aug 24, 2021

If the author of the PR deems the impact on product as negligible, approval by PM can be skipped.

I'm skipping PM approval.

@trentm trentm removed request for a team, russcam and gregkalapos August 24, 2021 17:38
…se not important or widely implemented enough, we can add it later if needed
@trentm trentm merged commit 535873f into elastic:master Aug 27, 2021
@trentm trentm deleted the long_field_max_length branch August 27, 2021 00:29
trentm added a commit to trentm/apm that referenced this pull request Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants