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

New feature that user assumpt an arbitrary value of sli when metrics hasn't a increase #242

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

casmssm
Copy link

@casmssm casmssm commented Sep 10, 2022

New feature that user assumpt an arbitrary value of sli when metrics hasn't a increase, and it is equals to 0. The metric exists but has no changes. Some scenarios we want a sli assumption to 100% or 0%. This feature can be used by adding the option assumption_empty_sli under spec in the slo_config.

We use this tool in production and we need this patch, but someone else might need it too.

CARLOS MARTINS and others added 5 commits September 9, 2022 20:35
…hasn't a increase, and it is equals to 0. The metric exists but has no changes. Some scenarios we want a sli assumption to 100% or 0%. This feature can be used by adding the option assumption_empty_sli under spec in the slo_config.
@lvaylet
Copy link
Collaborator

lvaylet commented Oct 17, 2022

Hi @casmssm,

I just took over the maintenance of the SLO Generator.

Could you give us a quick example where this extra setting is relevant, and what the corresponding YAML files look like?

Copy link
Collaborator

@ocervell ocervell left a comment

Choose a reason for hiding this comment

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

I suggest rewriting this in the following way to keep to code clean:

  • In the report code, add an if SLI value == NO_DATA and set the SLI value to DEFAULT_EMPTY_SLI_VALUE

nb: it might even makes sense to specify a default sli value per backend, but I'm not sure about the use cases.

data = self.run_backend(config, backend, client=client, delete=delete)
if not self._validate(data):
# Patch globo-0.3 for three retries
RETRIES=3
Copy link
Collaborator

@ocervell ocervell Oct 19, 2022

Choose a reason for hiding this comment

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

Retries do not belong here, this code should be left as clean as possible.
Retries should go in the providers (backends / exporters) code as they can be different for various backends. Please see https://github.com/google/slo-generator/blob/master/slo_generator/backends/dynatrace.py#L270 for an example.
On top of that, what is globo-0.3 ?

# Post validation
if not self._post_validate():
self.valid = False

def build(self, step, data):
def build(self, step, data, assumption_empty_sli=-1):
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, I don't think we should pass assumption_empty_sli to the build method.
imho the way should be a user configurable global variable to specify the SLI value when no events are found.
Please keep in mind that SLOs with no events cannot be relied on, read https://sre.google/workbook/alerting-on-slos/#low-traffic-services-and-error-budget-alerting for more info on why.

@@ -246,11 +280,18 @@ def get_sli(self, data):
if isinstance(data, tuple): # good, bad count
good_count, bad_count = data
if good_count == NO_DATA:
good_count = 0
#patch globo-0.3, terminate if not exists good events
good_count = 'EMPTY'
Copy link
Collaborator

Choose a reason for hiding this comment

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

good_count should be an int o a float, and NO_DATA already tells us that no events were found.

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