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

debug duplicate sla event ids #146

Merged
merged 19 commits into from
May 1, 2024

Conversation

fivetran-reneeli
Copy link
Contributor

@fivetran-reneeli fivetran-reneeli commented Mar 11, 2024

PR Overview

This PR will address the following Issue/Feature: #144

Note that this PR currently addresses the below scenarios that resulted in duplicate sla_event_ids, though please note the issue is not completely closed out yet:

  • 2 versions of the same schedule due to daylight savings time
  • overlapping schedule windows resulting from holidays that ran across more than 1 schedule week

The remaining root cause of duplicates we are still working to address is due to more than 1 condition in the filtered_reply_times CTE in the int_zendesk__reply_time_combined model being met (condition 2 in addition to condition 5). We have a working solution but still need to go through testing.

This PR will result in the following new package version: v0.14.1

Non breaking

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Bug Fixes

  • Fixes the issue of potential duplicate sla_event_id's occurring in the zendesk__sla_policies model.
    • This involved updating the int_zendesk__schedule_spine which was previously outputting overlapping schedule windows, to account for when holidays transcended a given schedule week.
    • This also involved updating the int_zendesk__reply_time_business_hours model, in which two different versions of a schedule could exist due to daylight savings time.
  • Improved performance by adjusting the int_zendesk__reply_time_business_hours model to only perform the weeks cartesian join on tickets that require the further look into the future.
    • Previously the int_zendesk__reply_time_business_hours would perform a cartesian join on all tickets to calculate weeks into the future. This was required to accurately calculate sla_elapsed_time for tickets with first replies far into the future. However, this was only necessary for a handful of tickets. Therefore, this has been adjusted to accurately only calculate the future weeks as far as either the first reply time or first solved time.

Documentation Updates

  • Addition of the reference to the Fivetran prebuilt Zendesk Streamlit report in the README.
  • Updates DECISIONLOG to include a note that the generated time series for ticket SLA policies is limited to a year into the future to maintain performance.

Bug Fixes

  • Fixes the issue of sla_event_id's occurring in the zendesk__sla_policies model.
    • This involved updating the int_zendesk__schedule_spine which was previously outputting overlapping schedule windows, to account for when holidays transcended a given schedule week.
    • This also involved updating the int_zendesk__reply_time_business_hours model, where within the model two different versions of a schedule would exist due to daylight savings time.

Documentation Updates

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

  • dbt run –full-refresh && dbt test
  • dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

  • The appropriate issue has been linked, tagged, and properly assigned
  • All necessary documentation and version upgrades have been applied
  • docs were regenerated (unless this PR does not include any code or yml updates)
  • BuildKite integration tests are passing
  • Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

  • this will be internally shared in our internal ticket as we tested on customer data
  • but customers have noted that the duplicates and the compute time have drastically decreased

If you had to summarize this PR in an emoji, which would it be?

💃

@fivetran-reneeli fivetran-reneeli changed the title cast from timestamp to date updates Mar 12, 2024
@fivetran-reneeli fivetran-reneeli changed the title updates debug duplicate sla event ids Mar 12, 2024
@fivetran-reneeli fivetran-reneeli linked an issue Apr 17, 2024 that may be closed by this pull request
1 task
@fivetran-reneeli fivetran-reneeli self-assigned this Apr 17, 2024
@fivetran-reneeli
Copy link
Contributor Author

regen docs when approved

* feature/first_reply_time_hours_performance_fix

* cleanup adjustments and documentation

* fix for databricks
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

@fivetran-reneeli I was able to do an initial review and just have two quick comments. Would you also be able to update the PR template with the most up to date findings and checks. Primarily so we can reference this in the future.

fivetran-reneeli and others added 3 commits May 1, 2024 15:20
…hours.sql

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
@fivetran-reneeli
Copy link
Contributor Author

Hi @fivetran-joemarkiewicz , this is ready for re-review.
Please give the updated PR template a read as well; I made sure to call out the 2 root causes of the duplicates that are addressed here. If that looks good I will make a similar comment in the issue thread so customers are aware.

Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

@fivetran-reneeli these changes look good to go from my end! My only request would be to make this a minor upgrade as opposed to a patch (v0.15.0). There are a number of changes in here that will change the results of the sla_policies end model (for the better), but will likely abruptly change users metrics. I would rather users safely upgrade themselves as opposed to seeing this sudden change out of the blue.

Would you be able to bump the version and then regen the docs. Then this will be good for release review.

Lastly, please be sure to update the initial issue once this is live and highlight the outstanding items which still need to be addressed.

@fivetran-catfritz fivetran-catfritz self-requested a review May 1, 2024 21:12
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz left a comment

Choose a reason for hiding this comment

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

Lgtm (pending buildkite)!

@fivetran-reneeli fivetran-reneeli merged commit c71cbe7 into main May 1, 2024
8 checks passed
@fivetran-reneeli fivetran-reneeli deleted the bugfix/duplicate_sla_event_ids_from_dst branch May 1, 2024 21:28
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.

[Bug] sla policy events failing due to duplicate values
3 participants