google / slo-generator

SLO Generator computes SLIs, SLOs, Error Budgets and Burn Rates from supported backends, then exports an SLO report to supported targets.
Apache License 2.0
489 stars 78 forks source link

šŸ› [BUG] - Cloud Monitoring MQL uses local timezone rather than UTC #329

Closed Lawouach closed 1 year ago

Lawouach commented 1 year ago

SLO Generator Version

2.3.3

Python Version

3.10

What happened?

I'm running a simple Cloud Monitoring MQL good/bad ratio query (actually using this SLO config https://github.com/google/slo-generator/issues/303) and while the query works really well. It shows an issue where it relies on the local timezone to generate the end time of the query.

But this means you miss your timezone difference in the datasets.

For instance, I'm in France so CET and I'm an hour difference from UTC now. If I set a 1 hour window, I basically do not get any result because it's in, the future from GCP's perspective.

If I switch my machine to a UTC timezone, then I do get data as expected.

What did you expect?

Get the query to use UTC timestamp to return the right dataset.

My guess is that on line https://github.com/google/slo-generator/blob/master/slo_generator/backends/cloud_monitoring_mql.py#L243 we could switch from:

end_time_str: str = datetime.fromtimestamp(timestamp)

to

end_time_str: str = datetime.fromtimestamp(timestamp, tz=timezone.utc)

Screenshots

![DESCRIPTION](LINK.png)

Relevant log output

$ sudo timedatectl set-timezone "Europe/Paris"
11:53 $ slo-generator compute --slo-config sloconf.yaml --config slogenconfig.yaml 
ERROR - cloudrun-service-availability    | 1 hour   | Not enough valid events (-1) found. Minimum valid events: 1.
INFO - Run finished successfully in 3.4s.
INFO - Run summary | SLO Configs: 1 | Duration: 3.4s
$ sudo timedatectl set-timezone UTC
$ slo-generator compute --slo-config sloconf.yaml --config slogenconfig.yaml 
INFO - cloudrun-service-availability    | 1 hour   | SLI: 72.3089 % | SLO: 85.0 % | Gap: -12.69% | BR: 1.8 / 9.0 | Alert: 0 | Good: 7275     | Bad: 2786    
INFO - Run finished successfully in 4.3s.
INFO - Run summary | SLO Configs: 1 | Duration: 4.3s

Code of Conduct

lvaylet commented 1 year ago

Hi @Lawouach, thanks a lot for reporting this behavior. Let me reproduce it locally on my machine. It looks related to #308 .

lvaylet commented 1 year ago

It is indeed the same issue and you both proposed similar fixes.

Just note I was not able to find a documentation for a fromtimestamp() method that accepts both a timestamp and a timezone. Is it something you have tested successfully yourself? If so, can you point me out to the official documentation? Otherwise I will probably lean toward Maxime's fix with .astimezone().

Lawouach commented 1 year ago

The signature of the function takes a tz argument:

If tz is not None, it must be an instance of a tzinfo subclass, and the timestamp is converted to tzā€™s time zone.

lvaylet commented 1 year ago

My bad. I was looking at the documentation for date and not datetime. Same URL but different sections.

lvaylet commented 1 year ago

Duplicate of #308

lvaylet commented 1 year ago

Closing with #308