Skip to content

fix: Add target ping interval & Add parameter validation for config#947

Closed
destinyoooo wants to merge 2 commits intoburningalchemist:masterfrom
destinyoooo:fix
Closed

fix: Add target ping interval & Add parameter validation for config#947
destinyoooo wants to merge 2 commits intoburningalchemist:masterfrom
destinyoooo:fix

Conversation

@destinyoooo
Copy link
Contributor

This PR includes the following improvements:

  1. Add target ping interval :

    • Added pingInterval field in target.go, defaulting to 30 seconds
    • Implemented ping caching mechanism, only ping the database when the last ping was more than pingInterval ago
    • Reduced unnecessary ping operations to the database, especially when using cached collectors
  2. Add parameter validation for config :

    • Enhanced checkRequiredFields method in config/config.go
    • Validated target configuration to ensure data_source_name and collectors fields exist
    • Validated jobs configuration to ensure job_name, collectors, static_configs, and targets fields exist

@burningalchemist
Copy link
Owner

Hey @destinyoooo, thank you for your contribution! 👍

Overall looks good, but do you mind splitting the PR in two? The scope is a bit different for these, and I'd like to keep changes scoped.

@destinyoooo
Copy link
Contributor Author

This PR includes the following improvements:

  1. Add target ping interval :

    • Added pingInterval field in target.go, defaulting to 30 seconds
    • Implemented ping caching mechanism, only ping the database when the last ping was more than pingInterval ago
    • Reduced unnecessary ping operations to the database, especially when using cached collectors
  2. Add parameter validation for config :

    • Enhanced checkRequiredFields method in config/config.go
    • Validated target configuration to ensure data_source_name and collectors fields exist
    • Validated jobs configuration to ensure job_name, collectors, static_configs, and targets fields exist

Of course, I have already completed it. In #948 and #949

@burningalchemist
Copy link
Owner

Thanks! 👍 Closing this one in favour of #948 and #949.

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.

2 participants