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

[REVERTED] feat: PC-13893 Deprecate usage of objective's value field for Composite SLOs #529

Merged
merged 12 commits into from
Sep 19, 2024

Conversation

ditrytus
Copy link
Contributor

@ditrytus ditrytus commented Aug 23, 2024

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

This change requires updates in the platform code which are in this PR:
https://github.com/nobl9/n9/pull/15176

The platform will first need to be released with unreleased version of SDK containing change in this PR, before SDK can be released.

Changes in terraform-provider-nobl9 depend on this change in SDK:

Testing

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.

@ditrytus ditrytus marked this pull request as draft August 23, 2024 11:39
@n9-machine-user n9-machine-user added enhancement New feature or request go minor labels Aug 23, 2024
@ditrytus ditrytus changed the title feat: PC-13893 objective value feat: PC-13893 objective's value is optional for Composite SLOs Aug 23, 2024
@ditrytus ditrytus changed the title feat: PC-13893 objective's value is optional for Composite SLOs feat: PC-13893 Deprecate objective's value field for Composite SLOs Aug 23, 2024
@ditrytus ditrytus changed the title feat: PC-13893 Deprecate objective's value field for Composite SLOs feat: PC-13893 Deprecate usage of objective's value field for Composite SLOs Aug 23, 2024
@ditrytus ditrytus marked this pull request as ready for review August 23, 2024 14:48
@ditrytus ditrytus changed the base branch from main to explicit-generic-arguments September 9, 2024 14:26
Base automatically changed from explicit-generic-arguments to main September 13, 2024 09:18
manifest/v1alpha/slo/slo.go Show resolved Hide resolved
@ditrytus ditrytus enabled auto-merge (squash) September 19, 2024 13:13
@ditrytus ditrytus merged commit 0350ad4 into main Sep 19, 2024
5 checks passed
@ditrytus ditrytus deleted the PC-13893-composite-value branch September 19, 2024 13:17
ditrytus added a commit that referenced this pull request Sep 20, 2024
ditrytus added a commit that referenced this pull request Sep 20, 2024
… Composite SLOs (#529)"

This reverts commit 0350ad4.

## Motivation

Changes we introduced in #529 and nobl9/n9#15176
are breaking API contract for older versions of SDK in a way that blocks
the release. We must prepare better backward compatibility before
introducing these changes.

## Summary

This reverses changes introduced in #529.

## Related changes

#529
nobl9/n9#15176

## Testing

## Release Notes

Changes from #529 are reverted.
ditrytus added a commit that referenced this pull request Sep 20, 2024
@ditrytus ditrytus changed the title feat: PC-13893 Deprecate usage of objective's value field for Composite SLOs [REVERTED] feat: PC-13893 Deprecate usage of objective's value field for Composite SLOs Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request go minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants