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

Warn or error on known bad rules configuration #566

Closed
cartermp opened this issue Nov 17, 2022 · 2 comments · Fixed by #701
Closed

Warn or error on known bad rules configuration #566

cartermp opened this issue Nov 17, 2022 · 2 comments · Fixed by #701
Labels
type: enhancement New feature or request

Comments

@cartermp
Copy link
Member

Similar to #559, but tracking separately...

Today, if you have a typo in rules config like Sample instead of Sampler, refinery will just sample all traces. That can lead to way more event volume than desired until it's fixed.

Ideally, if there's a known bad configuration like this, we could either warn or error on it.

@cartermp cartermp added the type: enhancement New feature or request label Nov 17, 2022
@kentquirk
Copy link
Contributor

I've done some investigation, and it seems like the right answer here is to leverage jsonschema, since YAML and TOML are freely convertible to and from JSON -- at least at the level we use in Refinery config.

I also think it's probably best done as a separate tool that someone can run separately, rather than embedding it into the refinery executable. (Although I'd hear arguments to the contrary!)

Writing the schema is not technically difficult, but is a bit tedious so is expected to take several days of work. It's now on the list of things to do soon-ish.

@seh
Copy link
Contributor

seh commented Nov 30, 2022

I maintain a schema in CUE that could be used for validation as well, if you'd like to see it. It's focused more on code generation than validation, as it's imbibed with the default values used by the Refinery application.

@kentquirk kentquirk added this to the v2.0 milestone Mar 22, 2023
@fchikwekwe fchikwekwe removed this from the v2.0 milestone Apr 24, 2023
kentquirk added a commit that referenced this issue Jun 6, 2023
…#701)

Note: this PR is dependent on #700 -- that one should be merged first.

## Which problem is this PR solving?

- Adds complete rules validation to the validation system, plus a few
bonus features.

## Short description of the changes

- Add a rulesMeta.yaml file to describe the rules data.
- Extend the validation system to handle a structure nested more deeply
than the basic config validation, which was only 2 layers deep. This
meant adding objects and arrays of objects.
- Add a few more validation features to support the new semantics.
- Add a "did you mean...?" feature to suggest the proper spelling if
someone gets something wrong. This is particularly useful when you get
the case wrong.
- Generate a .md file from the rules metadata; it's not yet pretty but
it's a good start. Will improve before release.

It's been validated against the converted rules_complete rules file. 

## Example results:
Rule snippet
```
    dataset4:
        RuleBasedSampler:
            Rules:
```
Validation: 
```
Validation Errors in rules file:
  Within sampler dataset4: unknown group RuleBasedSampler; did you mean RulesBasedSampler?
```

Rule snippet
```
Samplers:
    dataset1:
        DynamicSampler:
            ClearFrequency: 1
            SamplerRate: 10
            FieldList:
                - 10
                - request.method
                - http.target
                - response.status_code
            UseTraceLength: "true"
```
Validation: 
```
Validation Errors in rules file:
  Within sampler dataset1: field DynamicSampler.UseTraceLength must be a bool
  Within sampler dataset1: field DynamicSampler.ClearFrequency (1) must be a valid duration like '3m30s' or '100ms'
  Within sampler dataset1: unknown field DynamicSampler.SamplerRate; did you mean SampleRate?
  Within sampler dataset1: field DynamicSampler.FieldList must be a string array but contains non-string 10
  Within sampler dataset1: the group DynamicSampler is missing its required field SampleRate
```

I'm really happy with this -- it makes it almost impossible to make a
major error in your rules.

Next up: run the validators on Refinery startup.

Closes #566 
Closes #559 
Closes #518
Closes #273 
Closes #266
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants