[Bug] int_zendesk__schedule_spine - some work schedules not picking up holidays

cth84 commented 8 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Describe the issue

Looks like there was an edge case with the recent logic that went in, to fix holiday / business hours (again thank you for the help tackling that!)

We have a schedule whose business hours are 9:00-20:00 US/Eastern. This schedule_spine is still including the respective holidays as a day to be worked. However, a 9:00-17:00 US/Eastern schedule does not have this issue.

This appears to be due to the schedule's daily start and end times being converted to UTC but the start and end times created for the holidays, are not.

Relevant error log or model output

No response

Expected behavior

Would expect all schedules to pick up the appropriate holiday and drop that day from the schedule spine for business minutes evaluation

dbt Project configurations

Package versions

packages:

package: dbt-labs/dbt_utils version: 1.0.0
package: fivetran/fivetran_log version: 1.1.0
package: fivetran/jira version: 0.12.2
package: fivetran/github version: 0.7.0
package: dbt-labs/codegen version: 0.9.0
package: fivetran/zendesk version: 0.12.0

What database are you using dbt with?

bigquery

dbt Version

Core:

installed: 1.5.6

Plugins:

bigquery: 1.5.6

Additional Context

A comparison of two schedules, one that picks up Labor Day as non-worked and the other that does not

I believe this is fairly straight forward, due to the schedules daily start/end times being offset/converted to UTC:

But the start/end times for the Holiday (start of day, end of day) not being offset/converted to UTC as well:

And when the Holiday Check is applied, we don't set the holiday_name which, if set, is used to exclude the record downstream in the final cte:

Are you willing to open a PR to help address this issue?

[x] Yes.
[ ] Yes, but I will need assistance and will schedule time during our office hours for guidance
[ ] No.

fivetran-reneeli commented 8 months ago

Hi @cth84, thanks so much for not only opening this but leaving a detailed explanation with screenshots! That makes total sense and we should definitely account for these edge cases; great job noticing the UTC conversions. I agree the holiday times should also be converted to UTC.

Appreciate the PR as well! I left a comment asking about the change from second to a minute counter.

We will pick this up in an upcoming sprint!

cth84 commented 8 months ago

Thanks as always @fivetran-reneeli ! Followed up on the PR comment - feel free to reach out if there are any further questions/concerns. Clearly no harm if you toss it and approach it from scratch - just wanted to provide something tangible to start a convo with, since I got it working for our particular situation.

Oh and can definitely give you our schedules, holidays in csv format if you want to replicate this in the model - just let me know

fivetran-reneeli commented 8 months ago

I appreciate it Craig! It is handy to have a PR to start with. It won't hurt to get the schedules and holidays-- I believe we already have an email thread! Thank you 🙏

fivetran-avinash commented 8 months ago

Hi @cth! I'm picking up work on this task this sprint and hope to have a new release for this deployed in the coming weeks! Will be in touch and hopefully will have a branch for you to test next week.

fivetran-avinash commented 7 months ago

Hi @cth84 ! Thanks for your patience with us.

After investigating your proposed solve, we do agree that your particular fix does likely work for your case. However, it does not work for other cases, because the conversions you propose impact customers who are in time zones ahead of UTC. See more details in the PR comment.

Because we are crunched due to the holidays (and I will be going on PTO for most of Thanksgiving week), we will most likely not be deploying a fix for a few weeks. I'd love for you to take a look at the comment in your submitted PR and see if you can investigate a little bit more closely what logic could work to fit all cases for all time zones. We will still be working at this from our end to come up with a proper solution, but since you have better knowledge of this model, any insights you could provide would be invaluable.

Cheers and chat with you soon! Let us know if you have any questions as well.

fivetran-joemarkiewicz commented 7 months ago

Actually, @cth84 I believe we may have found a scalable solution! We were able to recreate the same issue you were experiencing and resolved it for cases where the timezone has either a + or - UTC offset. When you have availability, would you be able to test the following branch and let us know if this resolves the initial issue you raised here?

packages:
  - git: https://github.com/fivetran/dbt_zendesk.git
    revision: bugfix/holiday-check-utc-offset
    warn-unpinned: false

If you are curious, here is the adjustment we applied to resolve the issue in our testing. We used the same approach you had thought of to leverage the offset to convert to proper UTC, but applied the logic in a different cte and also used the same logic when converting the schedule to UTC in order to ensure both matched for downstream calculations. Let me know your thoughts!

fivetran-joemarkiewicz commented 7 months ago

@cth84 thanks again for working with our team. We believe we have addressed this issue in the latest version (v0.13.0) of the Zendesk package. Please feel free to upgrade and let us know if the issue has been addressed on your end. Thanks again for your contribution and working with our team to account for this issue in the package. 😄

fivetran / dbt_zendesk