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 configurable alert to print log statement warning of missing TIMs deposits #106

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

mcook42
Copy link

@mcook42 mcook42 commented Oct 10, 2024

PR Details

Description

We have added support for logging WARN level messages to indicate when we have not received any TIMs messages through our TimDepositController. The monitoring and the interval at which the monitor runs are both configurable via environment variables. I have also marked the executable scripts (like mvnw) as executable in the git index so that developers don't need to chmod +x the scripts whenever they want to run them.

Related Issue

No relevant GitHub Issue, but there is one for CDOT:

Modify ODE to log warning upon not receiving data for a configurable duration
https://dev.azure.com/SOC-OIT/CDOT/_workitems/edit/229495

Motivation and Context

As developers and maintainers of the ODE systems, we want a way to detect when we have lost connectivity or are not receiving data via the TimsDepositController. This will help us identify connectivity issues with WZDX and be proactive about resolving these issues.

How Has This Been Tested?

Unit tests were added where feasible to confirm code functions as intended. Where unit tests were not feasible, like in the case of log messages being emitted, local tests were run to simulate TIMs deposits and view the appropriate log messages. See the following screenshots of logs for more information:

  • No TIMs ingested scenario (first screenshot is the environment variable settings):
    Pasted image 20241010102333

Pasted image 20241010105841
Pasted image 20241010105925

  • TIMs ingested for 30 seconds then not ingested for 15 to show changing states over time are still tracked and alerted on (DEBUG level so these messages are not written to logs in production unless configured):

Pasted image 20241010110135
Pasted image 20241010110241
Pasted image 20241010110319

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
    ODE Contributing Guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@mcook42 mcook42 marked this pull request as ready for review October 10, 2024 23:02
Copy link
Member

@dmccoystephenson dmccoystephenson left a comment

Choose a reason for hiding this comment

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

Looks good, just a few questions!

sample.env Outdated Show resolved Hide resolved
Copy link

@mwodahl mwodahl left a comment

Choose a reason for hiding this comment

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

This all looks great! I just had a couple of questions

README.md Outdated Show resolved Hide resolved
sample.env Outdated Show resolved Hide resolved
jpo-cvdp Outdated Show resolved Hide resolved
Copy link
Member

@dmccoystephenson dmccoystephenson left a comment

Choose a reason for hiding this comment

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

The solution looks good overall, and the unit tests are passing! I just left a comment about a mismatch between the environment variable names in sample.env and docker-compose.yml, along with a suggestion to handle the empty string case for the variable.

sample.env Outdated Show resolved Hide resolved
Copy link

@mwodahl mwodahl left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@dmccoystephenson dmccoystephenson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

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.

3 participants