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

PC-10133: Move SLO into separate package #141

Merged
merged 105 commits into from
Nov 20, 2023

Conversation

nieomylnieja
Copy link
Collaborator

@nieomylnieja nieomylnieja commented Oct 3, 2023

This PR introduces:

  • Almost-complete rewrite of SLO validation.
  • Full test coverage of all the validated cases.
  • Moves SLO to its own package.
  • Major changes to validation library, namely:
    • New builtins
    • Intelligent pointer handling with ForPointer
    • More flexibility to errors handling, rules creation and Validator behaviour
  • Added ability to record tests, run:
    NOBL9_SDK_TEST_RECORD_FILE=/home/mh/test-output make test
    Make sure the path is absolute!

The error handling after these changes solidifies with the following inter object dependencies:

Untitled-2023-05-25-1306

The following benchmark was run to asses the performance of the solution:

var inputSLOs = func() []SLO {
	rec, err := os.Open("/home/mh/nobl9/recorded-tests.json")
	if err != nil {
		panic(err)
	}
	var slos []SLO
	if err = json.NewDecoder(rec).Decode(&slos); err != nil {
		panic(err)
	}
	return slos
}()

func BenchmarkValidate(b *testing.B) {
	for i := 0; i < b.N; i++ {
		for i := range inputSLOs {
			_ = validator.Check(inputSLOs[i])
		}
	}
}

With the following results for erroneous objects (around 240 of them):

  • old code:
    $ go test -bench=Validate -benchtime=10s -benchmem
    goos: linux
    goarch: amd64
    pkg: github.com/nobl9/nobl9-go/manifest/v1alpha
    cpu: 11th Gen Intel(R) Core(TM) i5-1145G7 @ 2.60GHz
    BenchmarkValidate-8         7450           1640675 ns/op         2093472 B/op      22081 allocs/op
    PASS
    ok      github.com/nobl9/nobl9-go/manifest/v1alpha      12.411s
  • new code:
    $ go test -bench=Validate -benchtime=10s -benchmem
    goos: linux
    goarch: amd64
    pkg: github.com/nobl9/nobl9-go/manifest/v1alpha/slo
    cpu: 11th Gen Intel(R) Core(TM) i5-1145G7 @ 2.60GHz
    BenchmarkValidate-8         2427           4949896 ns/op         1571118 B/op      35522 allocs/op
    PASS
    ok      github.com/nobl9/nobl9-go/manifest/v1alpha/slo  12.559s

This is expected, as there's much more function calls and details provided to errors.
Let's compare clean SLO:

  • old code:
    $ go test -bench=Validate -benchtime=10s -benchmem
    goos: linux
    goarch: amd64
    pkg: github.com/nobl9/nobl9-go/manifest/v1alpha
    cpu: 11th Gen Intel(R) Core(TM) i5-1145G7 @ 2.60GHz
    BenchmarkValidate-8       534976             21294 ns/op           11483 B/op         48 allocs/op
    PASS
    ok      github.com/nobl9/nobl9-go/manifest/v1alpha      11.637s
  • new code:
    $ go test -bench=Validate -benchtime=10s -benchmem
    goos: linux
    goarch: amd64
    pkg: github.com/nobl9/nobl9-go/manifest/v1alpha/slo
    cpu: 11th Gen Intel(R) Core(TM) i5-1145G7 @ 2.60GHz
    BenchmarkValidate-8       470991             25588 ns/op            9036 B/op         20 allocs/op
    PASS
    ok      github.com/nobl9/nobl9-go/manifest/v1alpha/slo  12.345s

Here, the difference is not as noticeable anymore, more so, the new code now allocates much less, but that also depends on an integration, I've run it only for Instana as it has a fairly complex validation.

FAQ:

Question might arise: how to do cross field checks?
Here a second question may appear: what is a cross field check?
Let's say we want to compare a value of a deeply nested field to a value of another field which resides in a different place on a struct properties tree. Say we want to make sure spec.indicator.metricSource.project is equal to metadata.project. The proposed validation DOES NOT allow reverse access to properties, you can only access your children. Hence the only way to achieve that is to simply create a rule on a higher level (in the example, on the top SLO level), which has the access to the properties in question and use WithName method to associate it with the downstream path of spec.indicator.metricSource.project, alternatively you can also return PropertyError from your custom top level validation rule.

Due to rebasing shenanigans the commits history is botched...

@nieomylnieja nieomylnieja changed the base branch from main to PC-10133-move-service-into-separate-package October 3, 2023 12:35
@nieomylnieja nieomylnieja force-pushed the PC-10133-move-service-into-separate-package branch 2 times, most recently from ae25dc0 to 1ccc047 Compare October 4, 2023 13:37
Base automatically changed from PC-10133-move-service-into-separate-package to main October 6, 2023 13:24
@nieomylnieja nieomylnieja force-pushed the PC-10133-move-slo-into-separate-package branch from bcccce7 to f2a61f6 Compare October 11, 2023 10:11
@nieomylnieja nieomylnieja changed the base branch from main to PC-10133-move-api-models-to-separate-package October 11, 2023 10:11
@nieomylnieja nieomylnieja force-pushed the PC-10133-move-slo-into-separate-package branch from f273a41 to 6390b6c Compare October 11, 2023 12:16
Base automatically changed from PC-10133-move-api-models-to-separate-package to main October 11, 2023 14:13
manifest/v1alpha/data_sources.go Outdated Show resolved Hide resolved
manifest/v1alpha/errors.go Outdated Show resolved Hide resolved
manifest/v1alpha/slo/metrics_graphite_test.go Outdated Show resolved Hide resolved
manifest/v1alpha/slo/metrics_instana.go Outdated Show resolved Hide resolved
manifest/v1alpha/slo/metrics_instana.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kskitek kskitek left a comment

Choose a reason for hiding this comment

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

Uh Oh! that's another big refactor

I like the new validation. It's very verbose but also very readable and easily adjustable (and testable!)

I also agree with the structure of Rules and Validators.

I didn't check the PR in details though and left just a few comments/questions.

.golangci.yml Outdated Show resolved Hide resolved
manifest/v1alpha/errors.go Outdated Show resolved Hide resolved
manifest/v1alpha/project/validation.go Outdated Show resolved Hide resolved
validation/errors.go Outdated Show resolved Hide resolved
Copy link
Member

@skrolikiewicz skrolikiewicz left a comment

Choose a reason for hiding this comment

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

Let's goo

@nieomylnieja nieomylnieja force-pushed the PC-10133-move-slo-into-separate-package branch from e9cde51 to df18924 Compare November 14, 2023 12:15
@nieomylnieja nieomylnieja merged commit f032fd3 into main Nov 20, 2023
3 checks passed
@nieomylnieja nieomylnieja deleted the PC-10133-move-slo-into-separate-package branch November 20, 2023 16:30
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