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

feat: allow loading rule-evaluator query options from config #1059

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheSpiritXIII
Copy link
Member

@TheSpiritXIII TheSpiritXIII commented Jul 8, 2024

Pre-req: #1104

The following are now settable in the rule-evaluator Prometheus config:

google_cloud:
  query:
    project_id: []string
    generator_url: string
    credentials: string

When both CLI arguments and configurations are present, the CLI arguments will be the default and the set configurations will override the CLI arguments.

@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch 10 times, most recently from f235b4a to b6573e9 Compare July 15, 2024 14:30
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch 3 times, most recently from 9487d00 to f2a7f9a Compare July 15, 2024 17:41
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch from f2a7f9a to efc2dc0 Compare July 17, 2024 19:57
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch 6 times, most recently from 5d46346 to f2f236b Compare August 5, 2024 18:05
@TheSpiritXIII TheSpiritXIII requested review from bwplotka and pintohutch and removed request for bwplotka August 5, 2024 18:07
@TheSpiritXIII TheSpiritXIII marked this pull request as ready for review August 5, 2024 18:08
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch 2 times, most recently from 74be4dd to d5b8f72 Compare August 8, 2024 01:59
@TheSpiritXIII TheSpiritXIII changed the base branch from main to TheSpiritXIII/rule-export-runtime August 8, 2024 02:47
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch 2 times, most recently from f1573e2 to fd6303c Compare August 8, 2024 03:07
@TheSpiritXIII TheSpiritXIII changed the title refactor: move some rule-evaluator flags to config feat: allow loading rule-evaluator query options from config Aug 8, 2024
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch 2 times, most recently from 420e694 to 2a7865a Compare August 8, 2024 03:15
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-export-runtime branch from f1f97f8 to 515d45b Compare August 8, 2024 03:19
}

return &evaluator, nil
}

func (e *ruleEvaluator) ApplyConfig(cfg *config.Config) error {
func (e *ruleEvaluator) ApplyConfig(cfg *config.Config, evaluatorOpts *evaluatorOptions) error {
if evaluatorOpts != nil && !reflect.DeepEqual(evaluatorOpts, e.lastEvaluatorOpts) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit of work that could be replaced by simply detecting change AND os.Exit(0) (with clear log AND metric ideally, AND do this in main ofc).

This has consequence of tricky to debug if the "restart" was actual crash or consequence of valid config changes, but potentially it's fine?

for {
// Copy the rule-manager before running, so we don't hold the lock.
e.mtx.Lock()
oldRuleManager := e.rulesManager
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will think about perhaps if there's anything to simplify here

@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-export-runtime branch from 515d45b to bcc7f6a Compare August 23, 2024 14:06
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch from 2a7865a to 3db475a Compare August 23, 2024 14:07
Base automatically changed from TheSpiritXIII/rule-export-runtime to main August 23, 2024 14:30
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch 2 times, most recently from 63d63a7 to 9d4a522 Compare August 23, 2024 14:42
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/rule-eval-runtime branch from 9d4a522 to 70b228c Compare August 23, 2024 16:11
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.

None yet

2 participants