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.

Sign up for GitHub

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

Jump to bottom

all: limit request tracer log count to five #8489

Merged
merged 2 commits into from Nov 15, 2023
Merged

all: limit request tracer log count to five #8489

merged 2 commits into from Nov 15, 2023

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Nov 14, 2023 •
edited

Proposed commit message

Currently the request tracer log is configured to retain all logs. This opens the risk of user filesystems being filled with unwanted logs. So limit the number of logs to five (picked as reasonably similar to the number of logs retained by elastic-agent, although the logs are not necessarily comparable in size or temporal domain).

Relevant docs for the change:

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@efd6 efd6 added enhancement New feature or request Team:Security-External Integrations Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team Team:Cloud-Monitoring Label for the Cloud Monitoring team Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team labels Nov 14, 2023
@efd6 efd6 self-assigned this Nov 14, 2023
@efd6 efd6 force-pushed the maxlogs_tracers branch 3 times, most recently from d3d7d1d to 80ae524 Compare November 14, 2023 01:28
@elasticmachine
Copy link

elasticmachine commented Nov 14, 2023 •
edited

💚 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: 2023-11-15T21:10:25.772+0000

  • Duration: 94 min 47 sec

Test stats 🧪

Test Results
Failed 0
Passed 2251
Skipped 3
Total 2254

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Nov 14, 2023 •
edited

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (272/272) 💚
Files 95.965% (333/347) ❕
Classes 95.965% (333/347) ❕
Methods 93.648% (3671/3920) ❕
Lines 91.409% (93762/102574) ❕
Conditionals 100.0% (0/0) 💚

@efd6
Copy link
Contributor Author

efd6 commented Nov 14, 2023

/test

2 similar comments
@efd6
Copy link
Contributor Author

efd6 commented Nov 14, 2023

/test

@efd6
Copy link
Contributor Author

efd6 commented Nov 14, 2023

/test

@efd6 efd6 marked this pull request as ready for review November 14, 2023 20:59
@efd6 efd6 requested review from a team as code owners November 14, 2023 20:59
@efd6 efd6 requested review from belimawr and rdner November 14, 2023 20:59
@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@elasticmachine
Copy link

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@ebeahan
Copy link
Member

ebeahan commented Nov 14, 2023 •
edited

So the configuration would now limit to 5 request tracer logs, each 1MB in size per the resource.tracer.maxsize default?

@efd6
Copy link
Contributor Author

efd6 commented Nov 14, 2023

Yes, that's approximately correct. There are a couple of integrations that limit log size to 5MB in their configs here, but all are now limited to 5 files.

ebeahan
ebeahan approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

Initially, I worried slightly about limiting to 5MB being too limited for certain troubleshooting situations. However, I think it's a sensible starting point and better than inadvertently filling disk if the toggle is turned on and forgotten. 😅

@efd6
Copy link
Contributor Author

efd6 commented Nov 15, 2023

/test

leehinman
leehinman approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to httpjson.yml.hbs look good, but I did notice 2 changelog.yml changes that looked odd.

packages/hashicorp_vault/changelog.yml Outdated
@@ -56,7 +56,7 @@
changes:
- description: Update package spec to 2.9.0.
type: enhancement
link: https://github.com/elastic/integrations/pull/1
link: https://github.com/elastic/integrations/pull/8489
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right PR number for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no it is not. I will repair that to what it should have been.

packages/o365/changelog.yml Outdated
@@ -36,7 +41,7 @@
changes:
- description: Fix mappings for dynamically mapped fields.
type: enhancement
link: https://github.com/elastic/integrations/pull/1
link: https://github.com/elastic/integrations/pull/8489
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right PR number for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

muthu-mps
muthu-mps reviewed Nov 15, 2023
View reviewed changes
Copy link
Contributor

@muthu-mps muthu-mps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are setting a default limit for the request tracer maxbackups. Can we update the documentation to have a mention about the default limit here?
Otherwise change looks good!

@elasticmachine
Copy link

Package qualys_vmdr - 0.8.0 containing this change is available at https://epr.elastic.co/search?package=qualys_vmdr

@elasticmachine
Copy link

Package rapid7_insightvm - 1.8.0 containing this change is available at https://epr.elastic.co/search?package=rapid7_insightvm

@elasticmachine
Copy link

Package salesforce - 0.12.0 containing this change is available at https://epr.elastic.co/search?package=salesforce

@elasticmachine
Copy link

Package sentinel_one - 1.19.0 containing this change is available at https://epr.elastic.co/search?package=sentinel_one

@elasticmachine
Copy link

Package slack - 1.17.0 containing this change is available at https://epr.elastic.co/search?package=slack

@elasticmachine
Copy link

Package snyk - 1.19.0 containing this change is available at https://epr.elastic.co/search?package=snyk

@elasticmachine
Copy link

Package sophos_central - 1.13.0 containing this change is available at https://epr.elastic.co/search?package=sophos_central

@elasticmachine
Copy link

Package spring_boot - 1.1.0 containing this change is available at https://epr.elastic.co/search?package=spring_boot

@elasticmachine
Copy link

Package symantec_edr_cloud - 0.3.0 containing this change is available at https://epr.elastic.co/search?package=symantec_edr_cloud

@elasticmachine
Copy link

Package system - 1.49.0 containing this change is available at https://epr.elastic.co/search?package=system

@elasticmachine
Copy link

Package tenable_io - 2.7.0 containing this change is available at https://epr.elastic.co/search?package=tenable_io

@elasticmachine
Copy link

Package tenable_sc - 1.20.0 containing this change is available at https://epr.elastic.co/search?package=tenable_sc

@elasticmachine
Copy link

Package ti_abusech - 1.24.0 containing this change is available at https://epr.elastic.co/search?package=ti_abusech

@elasticmachine
Copy link

Package ti_cif3 - 1.10.0 containing this change is available at https://epr.elastic.co/search?package=ti_cif3

@elasticmachine
Copy link

Package ti_cybersixgill - 1.25.0 containing this change is available at https://epr.elastic.co/search?package=ti_cybersixgill

@elasticmachine
Copy link

Package ti_maltiverse - 0.8.0 containing this change is available at https://epr.elastic.co/search?package=ti_maltiverse

@elasticmachine
Copy link

Package ti_misp - 1.27.0 containing this change is available at https://epr.elastic.co/search?package=ti_misp

@elasticmachine
Copy link

Package ti_opencti - 0.3.0 containing this change is available at https://epr.elastic.co/search?package=ti_opencti

@elasticmachine
Copy link

Package ti_otx - 1.22.0 containing this change is available at https://epr.elastic.co/search?package=ti_otx

@elasticmachine
Copy link

Package ti_rapid7_threat_command - 1.14.0 containing this change is available at https://epr.elastic.co/search?package=ti_rapid7_threat_command

@elasticmachine
Copy link

Package ti_recordedfuture - 1.20.0 containing this change is available at https://epr.elastic.co/search?package=ti_recordedfuture

@elasticmachine
Copy link

Package ti_threatq - 1.23.0 containing this change is available at https://epr.elastic.co/search?package=ti_threatq

@elasticmachine
Copy link

Package tines - 1.9.0 containing this change is available at https://epr.elastic.co/search?package=tines

@elasticmachine
Copy link

Package trellix_epo_cloud - 1.9.0 containing this change is available at https://epr.elastic.co/search?package=trellix_epo_cloud

@elasticmachine
Copy link

Package trend_micro_vision_one - 1.15.0 containing this change is available at https://epr.elastic.co/search?package=trend_micro_vision_one

@elasticmachine
Copy link

Package windows - 1.43.0 containing this change is available at https://epr.elastic.co/search?package=windows

@elasticmachine
Copy link

Package wiz - 0.4.0 containing this change is available at https://epr.elastic.co/search?package=wiz

@elasticmachine
Copy link

Package zeek - 2.22.0 containing this change is available at https://epr.elastic.co/search?package=zeek

@elasticmachine
Copy link

Package zerofox - 1.22.0 containing this change is available at https://epr.elastic.co/search?package=zerofox

@elasticmachine
Copy link

Package zeronetworks - 1.11.0 containing this change is available at https://epr.elastic.co/search?package=zeronetworks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmoog zmoog zmoog approved these changes

@ebeahan ebeahan ebeahan approved these changes

@leehinman leehinman leehinman approved these changes

@muthu-mps muthu-mps muthu-mps approved these changes

@belimawr belimawr Awaiting requested review from belimawr belimawr is a code owner automatically assigned from elastic/elastic-agent-data-plane

@rdner rdner Awaiting requested review from rdner rdner is a code owner automatically assigned from elastic/elastic-agent-data-plane

Assignees

@efd6 efd6

Labels
enhancement New feature or request Team:Cloud-Monitoring Label for the Cloud Monitoring team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@efd6 @elasticmachine @ebeahan @zmoog @leehinman @muthu-mps