Skip to content

Commit

Permalink
feat: PC-13893 Deprecate usage of objective's value field for Composi…
Browse files Browse the repository at this point in the history
…te SLOs (#549)

## Motivation

Currently, we have `value` field required for all objectives in an SLO.
We currently allow to not pass that field in YAML at all but it causes
it to default to 0 anyway and is returned `value: 0`. This is
inconsistent because GET yaml is different than APPLY YAML in such case.

Composite SLOs always have exactly one objective, so as such `value`
doesn’t matter for them at all as long as it doesn't change.

Moreover, the existence of value is documented and used in examples
which is confusing to new adopters of Composite SLOs because it is
required, changing it will restart budget, but it doesn’t do anything.

We want to encourage and allow not setting `value` field for Composite
SLOs while maintaining backward compatibility with users who perhaps
already explicitly set it.

## Summary

* `value` will be omitted if it is `null` in API.
* `value` field is omitted in all Composite SLO examples and E2E tests.

## Related changes

#551
#549
nobl9/n9#15406
nobl9/terraform-provider-nobl9#312
nobl9/terraform-provider-nobl9#295

## Testing

```go
package main

import (
	"context"
	"github.com/nobl9/nobl9-go/manifest"
	"github.com/nobl9/nobl9-go/manifest/v1alpha/service"
	"github.com/nobl9/nobl9-go/manifest/v1alpha/slo"
	"github.com/nobl9/nobl9-go/manifest/v1alpha/twindow"
	"github.com/nobl9/nobl9-go/sdk"
	v1 "github.com/nobl9/nobl9-go/sdk/endpoints/objects/v1"
	"log"
	"time"
)

func main() {
	ctx := context.Background()
	client, err := sdk.DefaultClient()
	if err != nil {
		log.Fatalf("failed to create sdk client, err: %v", err)
	}
	const project = "value-test"
	svc := service.New(service.Metadata{Name: "my-service", Project: project}, service.Spec{})
	composite := slo.New(slo.Metadata{Name: "my-slo", Project: project}, slo.Spec{
		BudgetingMethod: slo.BudgetingMethodOccurrences.String(),
		Objectives: []slo.Objective{
			{
				ObjectiveBase: slo.ObjectiveBase{
					Name: "objective-1",
					// Look MA! No value!
				},
				BudgetTarget: ptr(0.99),
				Composite: &slo.CompositeSpec{
					MaxDelay: time.Hour.String(),
					Components: slo.Components{
						Objectives: []slo.CompositeObjective{},
					},
				},
			},
		},
		Service: svc.Metadata.Name,
		TimeWindows: []slo.TimeWindow{
			{
				Unit:      twindow.Day.String(),
				Count:     1,
				IsRolling: true,
			},
		},
	})
	objects := []manifest.Object{svc, composite}
	if err := manifest.Validate(objects); err != nil {
		log.Fatalf("failed to validate manifest, err: %v", err)
	}

	if err := client.Objects().V1().Apply(ctx, objects); err != nil {
		log.Fatalf("failed to apply objects, err: %v", err)
	}

	slos, err := client.Objects().V1().GetV1alphaSLOs(ctx, v1.GetSLOsRequest{
		Names:   []string{composite.GetName()},
		Project: project,
	})
	if err != nil {
		log.Fatalf("failed to get slo, err: %v", err)
	}

	if slos[0].Spec.Objectives[0].Value != nil {
		log.Fatalf("expected nil, got %v", *slos[0].Spec.Objectives[0].Value)
	}

	log.Println("Composite SLO's value is nil")
}

func ptr[T any](t T) *T {
	return &t
}

```

## Release Notes

Usage of `spec.objective[0].value` field for Composite SLOs becomes
deprecated.
* New Composite SLOs should not set `value` field.
* New Composite SLOs will still accept `value: 0` for backward
compatibility with older versions of Nobl9 SDK and Nobl9 Terrafrom
Provider.
* If `value` was previously set to `0` for Composite SLO then it should
be omitted going forward.
* If `value` was previously set in Composite SLO to a number other than
`0` then it can no longer be updated but still will be accepted for
backward compatibility.

The usage of `value` for SLOs using ratio or threshold SLIs does not
change.
  • Loading branch information
ditrytus authored Dec 5, 2024
1 parent 4824950 commit 797d83f
Show file tree
Hide file tree
Showing 4 changed files with 1 addition and 7 deletions.
1 change: 0 additions & 1 deletion internal/manifest/v1alpha/examples/slo_composite.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ func (s sloCompositeExample) SLO() v1alphaSLO.SLO {
{
ObjectiveBase: v1alphaSLO.ObjectiveBase{
DisplayName: "User experience",
Value: ptr(0.0),
Name: "user-experience",
},
BudgetTarget: ptr(0.95),
Expand Down
4 changes: 0 additions & 4 deletions manifest/v1alpha/slo/examples/composite-slo.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
budgetingMethod: Occurrences
objectives:
- displayName: User experience
value: 0.0
name: user-experience
target: 0.95
composite:
Expand Down Expand Up @@ -96,7 +95,6 @@
budgetingMethod: Occurrences
objectives:
- displayName: User experience
value: 0.0
name: user-experience
target: 0.95
composite:
Expand Down Expand Up @@ -160,7 +158,6 @@
budgetingMethod: Timeslices
objectives:
- displayName: User experience
value: 0.0
name: user-experience
target: 0.95
timeSliceTarget: 0.9
Expand Down Expand Up @@ -228,7 +225,6 @@
budgetingMethod: Timeslices
objectives:
- displayName: User experience
value: 0.0
name: user-experience
target: 0.95
timeSliceTarget: 0.9
Expand Down
2 changes: 1 addition & 1 deletion manifest/v1alpha/slo/slo.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ type Attachment struct {
// ObjectiveBase base structure representing an objective.
type ObjectiveBase struct {
DisplayName string `json:"displayName"`
Value *float64 `json:"value"`
Value *float64 `json:"value,omitempty"`
Name string `json:"name"`
NameChanged bool `json:"-"`
}
Expand Down
1 change: 0 additions & 1 deletion sdk/test_data/reader/expected/composite_v2_slo.tpl.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
"objectives": [
{
"displayName": "composite-obj",
"value": null,
"name": "composite-obj",
"target": 0.99,
"composite": {
Expand Down

0 comments on commit 797d83f

Please sign in to comment.