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

[RAM] Snooze end time shows 1 day ahead of actual end time #152630

Closed
JiaweiWu opened this issue Mar 3, 2023 · 5 comments · Fixed by #152873 or #153972
Closed

[RAM] Snooze end time shows 1 day ahead of actual end time #152630

JiaweiWu opened this issue Mar 3, 2023 · 5 comments · Fixed by #152873 or #153972
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Alerting/RulesManagement Issues related to the Rules Management UX Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@JiaweiWu
Copy link
Contributor

JiaweiWu commented Mar 3, 2023

From the SDH: https://github.com/elastic/sdh-kibana/issues/3588

A user noticed their snooze end time is 1 day after the actual end time. The user is in the CET timezone, which is GMT+1 I am able to reproduce this issue using a unit test. In the test below I have simulated the date that was reported in the SDH:

import { getRuleSnoozeEndTime } from './is_rule_snoozed';
import moment from 'moment';

describe('repro', () => {
  // To reproduce, first we must set the timezone to the customer's timezone, 
  // In alerting/jest.config.js, before module.exports, we can set the timezone:
  // 
  // process.env.TZ = 'CET';

  it('start', () => {
    // Set the current time as: 
    //   - Feb 27 2023 08:15:00 GMT+0000
    jest
      .useFakeTimers()
      .setSystemTime(new Date('2023-02-27T08:15:00.000Z'));

    // Try to get snooze end time with:
    //   - Start date of: Feb 24 2023 23:00:00 GMT+0000
    //   - End date of: Feb 27 2023 06:00:00 GMT+0000
    //     - Which is obtained from start date + 2 days and 7 hours (198000000 ms) 
    const result = getRuleSnoozeEndTime({
      muteAll: false,
      snoozeSchedule: [
        {
          "duration": 198000000,
          "rRule": {
            "byweekday": [
              "SA"
            ],
            "tzid": "Europe/Madrid",
            "freq": 2,
            "interval": 1,
            "dtstart": "2023-02-24T23:00:00.000Z"
          },
          "id": "9141dc1f-ed85-4656-91e4-119173105432"
        }
      ]
    });
    
    // In theory, the snooze should be over now, this should pass but it does not.
    expect(result).toEqual(null);

    // Instead we get: 
    //
    // expect(received).toEqual(expected) // deep equality
    // Expected: null
    // Received: 2023-02-28T05:00:00.000Z
    //

    // If we try to log what we see on the front end snooze tooltip, we get the 
    // same bug that the customer noticed (snooze for a day)
    //
    // console.log(moment(result).fromNow(true));
  });
});

I looked in our snooze code and I think the before call from is_snooze_active line 43 is giving us the wrong result:

const lastOccurrence = recurrenceRule.before(new Date(now), true);
@JiaweiWu JiaweiWu added bug Fixes for quality problems that affect the customer experience Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/RulesManagement Issues related to the Rules Management UX labels Mar 3, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@Zacqary
Copy link
Contributor

Zacqary commented Mar 7, 2023

Just started investigating but this is probably related to the fact that this rule is supposed to start on Friday February 24th and recur every Saturday.

It’s checking for the end of the “last occurrence” when a new occurrence begins one hour after the snooze is supposed to start. We’ve got two overlapping occurrences.

@Zacqary
Copy link
Contributor

Zacqary commented Mar 7, 2023

Yeah so shifting the start time forward by an hour makes this pass successfully. Seems like there's an issue related to time zones not taking days of the week into account correctly, I'll work on a fix.

@Zacqary
Copy link
Contributor

Zacqary commented Mar 7, 2023

Test also fails if we set the date to March 6th UTC. It looks like RRule isn't converting dtstart times to the provided tzid

According to jkbrzt/rrule#501 this may actually be the correct behavior, as per the spec? But the library doesn't follow the spec? I'm not sure. Seems like the library is actually following the spec as far as weekdays are concerned

@Zacqary
Copy link
Contributor

Zacqary commented Mar 7, 2023

Okay so I have found out what's happening. rrule.js does not respect the Z part of ISO date strings. Its only understanding of timezone is from the tzid and throws out any timezone information that's included in dtstart.

I think this is actually RRule spec compliant, it's just super weird in Javascript. We basically need to treat the Date objects that rrule.js generates for us as if they're strings in the local timezone, and we can't rely on them for accurate unix timestamps.

@JiaweiWu JiaweiWu moved this from In Progress to In Review in AppEx: ResponseOps - Rules & Alerts Management Apr 3, 2023
JiaweiWu added a commit that referenced this issue Apr 6, 2023
## Summary
Resolves: #152630

For maintenance windows, I noticed I was getting inconsistent unit test
results depending on the timezone of the computer that is running the
tests. To fix this I normalized the inputs going into RRule. I realized
I could apply this to the snooze bug, and lo and behold this change seem
to get us consistent results for the snooze tests and fixes the timezone
issue we had.

This works as a temporary fix for the snooze bug and making maintenance
window test consistent across different CI regions, however we will want
to migrate to our own RRule library
(#152873) once we can test it
more.

## Tested in the following timezones:

```
// in jest.config.ts
//
// process.env.TZ = 'America/Los_Angeles'
// process.env.TZ = 'America/Denver'
// process.env.TZ = 'America/New_York'
// process.env.TZ = 'UTC'
// process.env.TZ = 'Europe/London'
// process.env.TZ = 'Europe/Madrid'
// process.env.TZ = 'Asia/Calcutta'
// process.env.TZ = 'Asia/Tokyo'
```

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Zacqary added a commit that referenced this issue Jul 2, 2023
…pliant lib (#152873)

## Summary

Closes #152630

~Adds a fix for the weird UTC-but-not-really expected inputs in
rrule.js~

This PR removes the third-party `rrule` package and replaces it with
`@kbn/rrule`.

The third party RRule library's functions produced different results
depending on what system timezone you ran it in. It would output local
timestamps in UTC, making it impossible to do reliable math on them.
It's now replaced with our own library that passes all of our own tests
for the limited cross-section of the RRule spec that we need to support.
It's possible that it wouldn't stand up to the rigor of more complex
RRule queries, but it supports the ones that our Recurrence Scheduler UI
supports just fine.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Alerting/RulesManagement Issues related to the Rules Management UX Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
No open projects
3 participants