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 warnings to setup.template.overwrite #4463

Closed
wants to merge 1 commit into from
Closed

Conversation

Leaf-Lin
Copy link

Motivation/summary

Similar to elastic/beats#22357, setup.template.overwrite could potentially overload Elasticsearch with too many update requests.
On Elasticsearch side, it will be addressed in newer versions by introducing no-op updates which will be available from 7.11+ elastic/elasticsearch#64493

See also elastic/elasticsearch#57662

Thought having some warnings in the APM setting here should prevent users unwittingly leave template auto-creation on across large number of APM.

Checklist

I have considered changes for:

Similar to elastic/beats#22357, setup.template.overwrite could potentially overload Elasticsearch with too many update requests. 
On Elasticsearch side, it will be addressed in newer versions by introducing no-op updates which will be available from 7.11+ elastic/elasticsearch#64493

See also elastic/elasticsearch#57662

Thought having some warnings in the APM setting here should prevent users unwittingly leave template auto-creation on across large number of APM.
@apmmachine
Copy link
Contributor

💚 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

Expand to view the summary

Build stats

  • Build Cause: Pull request #4463 opened

  • Start Time: 2020-11-27T04:15:51.170+0000

  • Duration: 41 min 32 sec

Test stats 🧪

Test Results
Failed 0
Passed 4754
Skipped 143
Total 4897

Steps errors 3

Expand to view the steps failures

Compress

  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage

Compress

  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests

Test Sync

  • Took 3 min 8 sec . View more details on here
  • Description: ./.ci/scripts/sync.sh

@axw
Copy link
Member

axw commented Nov 30, 2020

Thanks @Leaf-Lin! I imagine that because APM Server is (currently) mostly deployed in smaller numbers, it wouldn't be such an issue. Once we move to running under Elastic Agent, on the edge, that would change; but we'll also be moving template management to Fleet then. Anyway, doesn't hurt to be cautious here.

@bmorelli25 I'm not sure how/when we normally update these copied-from-beats docs, so I'll leave it to you.

@axw axw requested a review from bmorelli25 November 30, 2020 01:10
@axw axw added the docs label Nov 30, 2020
@bmorelli25
Copy link
Member

Thanks, @Leaf-Lin!

This documentation is shared by both Beats and APM Server, with Beats being the source of truth. It looks like Naomi added this note to the Beats documentation recently, but we haven't updated the file on the APM side. Because APM Server programmatically updates documentation from Beats, committing this PR would only be temporary -- the contents would be overwritten on our next update.

I've opened elastic/beats#22804 to address a few typos in the original PR. Once that is merged, I'll pull the changes into APM Server. In the meantime, I'll close this PR. Thanks again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants