Give wiggle room for selecting SLA policy names if timestamps are off by milliseconds #174
Conversation
There was a problem hiding this comment.
@fivetran-jamie thanks for working so quick to address this issue. I tested these changes and they look good to me. I do have a suggestion to make some adjustments to the validation tests to ensure the either don't run or ignore the business metrics when the using_schedules variable is disabled.
If you could make those updates so we could use these validations in the future that would be great. Once those are applied, you can move this forward to release review. Thanks again!
CHANGELOG.md
Outdated
| - Addressed an issue in which some records in `zendesk__sla_policies` might erroneously have a null `sla_policy_name` due to system-generated millisecond-long gaps in timestamps. The package now compares timestamps to the nearest `second` when selecting valid SLA policy names in [int_zendesk__sla_policy_applied](https://github.com/fivetran/dbt_zendesk/blob/main/models/sla_policy/int_zendesk__sla_policy_applied.sql). | ||
|
|
||
| ## Under the Hood | ||
| - Updated `consistency_sla_policies` data validation test to account for the above change. |
There was a problem hiding this comment.
We can probably combine the two sentences here into one, I think they were both to account for the above changes?
| max(sla_elapsed_time) over (partition by ticket_id, metric, sla_applied_at) as max_sla_elapsed_time, | ||
| min(sla_elapsed_time) over (partition by ticket_id, metric, sla_applied_at) as min_sla_elapsed_time, | ||
|
|
||
| {# |
There was a problem hiding this comment.
Reminder to remove before merging!
| max_sla_elapsed_time - min_sla_elapsed_time > 2 | ||
|
|
||
| {# Remove after v0.18.1 #} |
There was a problem hiding this comment.
Reminder to remove before merging!
There was a problem hiding this comment.
@fivetran-jamie One question (see here) and a few minor comments before approving.
PR Overview
This PR will address the following Issue/Feature:
Customer has reported seeing records in
zendesk__sla_policieswith nullsla_policy_namevalues when they should not be null. We've isolated this problem to this join. It turns out that some records can come from Zendesk in the wrong order by a margin of a couple of milliseconds, which our current join logic is too strict to include.This PR will result in the following new package version:
v0.18.1
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Bug Fix
zendesk__sla_policiesmight erroneously have a nullsla_policy_namedue to system-generated millisecond-long gaps in timestamps. The package now compares timestamps to the nearestsecondwhen selecting valid SLA policy names in int_zendesk__sla_policy_applied.Under the Hood
consistency_sla_policiesdata validation test to account for the above change.sla_count_matchdata validation test.PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
Added
fivetran_integrity_sla_count_match_ticketsexclusion tickets related to #175 and validation tests are passing using both the testing and prod schemas:Regarding the
add_exceptionsCTE I added to theconsistency_sla_policiestest - I added thename_was_null_priorclause because there were ~5K tickets in our internal dataset that had their null SLA policy names updated by my fix.If you had to summarize this PR in an emoji, which would it be?
🪟