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

bugfix/sla-investigations #136

Merged
merged 30 commits into from
Feb 21, 2024
Merged

Conversation

fivetran-joemarkiewicz
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz commented Jan 29, 2024

PR Overview

This PR will address the following Issue/Feature: Issue #142, Issue #139, Issue #135, Issue #131, Issue #116

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

While this is not technically a breaking change. There are a number of changes in this update that will result in new and changed records. I would rather we make this breaking to highlight the large number of changes so users are aware of what changes will be applied and the impacts before upgrading.

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

Bug Fixes

  • Converted the zendesk__sla_policies metric for sla_elapsed_time to be reported in minutes to the second as opposed to just the nearest rounded minute. This ensures more accurate reporting.
  • Updated the int_zendesk__reply_time_combined model to additionally account for the following business hour scenarios as they were erroneously being filtered out in previous versions of the package:
    • A ticket is first replied to outside SLA schedules
    • A ticket is has not yet received an agent reply
  • Included additional logic within the int_zendesk__ticket_schedules model to more accurately select the active default schedule.
    • Previously the model could possibly select a deleted schedule. This update ensures only an active schedule is selected.
  • Overhauled the logic used to calculate sla_breach_at within the zendesk__sla_policies and upstream models for reply time SLAs. It was found this field was inconsistent with the actual breach/achieve time of an SLA. The overhaul should now ensure reply time SLA is accurate to either be the time of the SLA breach or achieve event.
  • Modified the logic that matches schedule weeks when calculating reply time business metrics. Previously long running SLAs would be excluded from the final model, now all reply time business SLAs regardless of sla elapsed duration will be included in the end zendesk__sla_policies model.

Documentation Updates

  • Updated "Zendesk" references within the README to now refer to "Zendesk Support" in order to more accurately reflect the name of the Fivetran Zendesk Support Connector.
  • Added new entries to the DECISIONLOG to highlight nuances and opinionated stances this package uses when calculating business metrics and first_reply_time SLAs.

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
  • [Complete docs will be regenerated following approval] 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:

Please see the height ticket for validations

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

📆

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@fivetran-reneeli fivetran-reneeli left a comment

Choose a reason for hiding this comment

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

Hi @fivetran-joemarkiewicz this looks great and nice job detailing all of it in depth in the changelog! The validations are really helpful as well. I want to comb through this one more time with a fresh brain tomorrow morning, but here are some notes I caught this first time around.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@fivetran-reneeli
Copy link
Contributor

Hey @fivetran-joemarkiewicz I was able to finish my review! This looks pretty good and the validations were very thorough 👍 . One suggestion for the changelog is it would help with navigating the changes if each bullet point was set up like 'Updated x model for the y metric / z logic', just so it's easier to track.

Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>
@fivetran-joemarkiewicz
Copy link
Contributor Author

Thanks for the suggestions @fivetran-reneeli! I just applied them and regenerated the docs. Re-requesting review. Let me know if there is anything else 😄

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>
Copy link
Contributor

@fivetran-reneeli fivetran-reneeli left a comment

Choose a reason for hiding this comment

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

Looks good! just caught a small typo in the changelog but otherwise good to go

@fivetran-joemarkiewicz
Copy link
Contributor Author

Looks good! just caught a small typo in the changelog but otherwise good to go

Thanks so much @fivetran-reneeli! I really appreciate the review and great notes for improving the documentation. Moving this forward in the release process! 🎉

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