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 reduce lastvalid timestamp warn logs frequency #1011

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Conversation

ymtaye
Copy link
Contributor

@ymtaye ymtaye commented Jan 17, 2024

Description of the issue

CloudWatch Agent introduced a warning log when timestamp falls back to an older valid timestamp #870
This log isn't throttled and can produce excessive warning logs when timestamp isn't parsing

Description of changes

Set the warning log message to print per set interval time

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Manually tested confirmed warning log prints per set interval (default 5 minutes)
Screenshot 2024-01-30 at 4 51 56 PM

Requirements

Before commit the code, please do the following steps.

  1. Run make fmt and make fmt-sh
  2. Run make lint

@ymtaye ymtaye marked this pull request as ready for review January 17, 2024 23:22
@ymtaye ymtaye requested a review from a team as a code owner January 17, 2024 23:22
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2024

Codecov Report

Attention: 42 lines in your changes are missing coverage. Please review.

Comparison is base (96d4763) 57.58% compared to head (214ef5a) 63.47%.
Report is 480 commits behind head on main.

Files Patch % Lines
cfg/aws/credentials.go 0.00% 22 Missing ⚠️
cfg/aws/shared_config.go 62.50% 15 Missing ⚠️
cfg/envconfig/envconfig.go 50.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1011      +/-   ##
==========================================
+ Coverage   57.58%   63.47%   +5.89%     
==========================================
  Files         370      364       -6     
  Lines       17548    18726    +1178     
==========================================
+ Hits        10105    11887    +1782     
+ Misses       6848     6220     -628     
- Partials      595      619      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jefchien
Copy link
Contributor

I don't see any new unit tests. Can you build the agent and verify that the sampling works as intended?

Copy link
Contributor

This PR was marked stale due to lack of activity.

@github-actions github-actions bot added the Stale label Jan 26, 2024
@ymtaye ymtaye changed the title Add zap sampling to reduce timestamp warn logs Add reduce lastvalid timestamp warn logs frequency Jan 30, 2024
if time.Since(p.lastUpdateTime) > warnOldTimeStamp {
p.Log.Warnf("Unable to parse timestamp, using last valid timestamp found in the logs %v: which is at least older than 1 day for log group %v: ", p.lastValidTime, p.Group)
{
if time.Since(p.lastWarnMessage) > logWarnInterval {
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine check with the one above.

@@ -22,6 +22,7 @@ const (
reqSizeLimit = 1024 * 1024
reqEventsLimit = 10000
warnOldTimeStamp = 1 * 24 * time.Hour
logWarnInterval = 1 * 5 * time.Minute
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would rename this to make it clearer that it's related to the old timestamp warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to
warnOldTimeStampLogInterval = 1 * 5 * time.Minute

@github-actions github-actions bot removed the Stale label Jan 31, 2024
if time.Since(p.lastUpdateTime) > warnOldTimeStamp {
p.Log.Warnf("Unable to parse timestamp, using last valid timestamp found in the logs %v: which is at least older than 1 day for log group %v: ", p.lastValidTime, p.Group)
// Check when timestamp has an interval of 1 days.
if (time.Since(p.lastUpdateTime) > warnOldTimeStamp) && (time.Since(p.lastWarnMessage) > warnOldTimeStampLogInterval) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Don't need the parentheses.

@ymtaye ymtaye merged commit d8d8a8e into aws:main Jan 31, 2024
6 checks passed
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.

4 participants