Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#2781 closed enhancement (fixed)

Exception report framework to replace tests that halt ETL

Reported by: dconnolly Owned by: ngraham
Priority: major Milestone: heron-jamestown-update
Component: data-repository Keywords: etl-ui public-web
Cc: ngraham, mhaog Blocked By:
Blocking: 3207, 3395, 3421, 3536 Sensitive: no

Description

From HeronProjectTimeline#May2014Planning. This includes way to replace the "divide by zero" tests that are all over our ETL code.

Change History (48)

comment:1 Changed 3 years ago by ngraham

  • Milestone changed from heron-hackberry-update to heron-wakarusa-update
  • Owner changed from mhoag to dconnolly
  • Status changed from new to assigned

comment:2 Changed 3 years ago by dconnolly

  • Blocking set to 2792

(In #2792) A few more ETL blockers.

comment:3 Changed 3 years ago by ssuman

  • Blocking 2792 deleted

(In #2792) Removed (#2781) from blocking the ETL for HERON Hackberry release
because its milestone changed from heron-hackberry-update to heron-wakarusa-update.

comment:4 Changed 3 years ago by dconnolly

  • Owner changed from dconnolly to ngraham
  • Priority changed from major to minor

comment:5 Changed 3 years ago by ngraham

  • Milestone changed from heron-delaware-update to heron-saline-update

Null medication ids falls in to this category - see ticket:2925#comment:7.

comment:6 Changed 3 years ago by dconnolly

  • Milestone changed from heron-verdigris-update to heron-kanopolis-update

postponing tickets not identified as Verdigris priorities in today's heron-weekly meeting

comment:7 Changed 3 years ago by dconnolly

  • Milestone changed from heron-kanopolis-update to dconnolly

Milestone renamed

comment:8 Changed 3 years ago by dconnolly

  • Milestone changed from dconnolly to heron-kanopolis-update

Milestone renamed

comment:9 Changed 3 years ago by dconnolly

  • Milestone changed from heron-kanopolis-update to dconnolly

Milestone renamed

comment:10 Changed 3 years ago by dconnolly

  • Milestone changed from dconnolly to heron-kanopolis-update

Milestone renamed

comment:11 Changed 3 years ago by dconnolly

  • Milestone changed from heron-kanopolis-update to heron-mcmillan-marsh-update
  • Priority changed from minor to major

scheduled for June in HeronProjectTimeline#Jan2015Planning

using the latest HERON milestone we have on the roadmap for now

comment:12 Changed 3 years ago by ngraham

  • Blocking set to 3309

comment:13 Changed 3 years ago by mhoag

I made a report (and a sketch framework for reporting) for UHC Mapping counts relative to the number of actual UHC encounters in the source system (see: ticket:3172#comment:7)

comment:14 Changed 3 years ago by ngraham

  • Keywords heron-weekly added

Adding heron-weekly as this is at risk for ETL starting this Friday.

comment:15 Changed 3 years ago by dconnolly

  • Blocking changed from 3309 to 3207, 3309

comment:16 Changed 3 years ago by dconnolly

  • Milestone changed from heron-mcmillan-marsh-update to heron-cheyenne-bottoms-update

Postponing this concerns me w.r.t. the April upgrade to clarity 2014 (#3207), but I suppose it's not our April release but rather May when we have to be ready for it.

comment:17 Changed 3 years ago by dconnolly

  • Blocking changed from 3207, 3309 to 3207

sorry for the noise; neglected to un-block March ETL

comment:18 Changed 3 years ago by ngraham

  • Keywords heron-weekly removed

comment:19 Changed 3 years ago by mhoag

  • Owner changed from ngraham to mhoag

Trading with Nathan (he is taking #3082)

comment:20 Changed 3 years ago by ngraham

  • Blocking changed from 3207 to 3207, 3394

comment:21 Changed 3 years ago by ngraham

  • Owner changed from mhoag to ngraham
  • Status changed from assigned to accepted

Plan of action resulting in discussion between NG, MH, DC:

  • Matt find the first place the Clarity 2014 ETL falls over (#3207).
  • Nathan rewrite the test in a way that allows the ETL to "trundle on" rather than fall over.
  • Lather, rinse, repeat...

Design discussion with Dan, Matt, Nathan:

  • More "User facing columns" like "which domain" (top level folder)
  • Concept paths/codes - some way to lead Tamara (or someone) to what the test actually is.
    • "if this exception happens, what does that mean to the customer"?
    • DC: "Talond?" had nifty structured reporting. Could have numerator/denominator/percent.
      • Report value might be a percent, but "2 percent of what" for diagnosis of the failures
  • MH: Counting as a whole - what are we counting? Patients?
    • DC: patients, facts, percent - note the "units"

DC: problem reports or success reports? Tend to be coded already as problem reports. 0 is happy.
MH/DC: Domain could be provided by curated data.
DC: Minimum user result: SQL table. Next step might be CSV dump as an artifact.

comment:22 Changed 3 years ago by ngraham

Matt and I made progress on the exception report on the exception_report_2781 branch. We added a couple of example tests and also ported a real-world test ([60542488c400]) that was causing our Clarity 2014 ETL to fail (#2781).

Here's some example report output:

TEST_RESULTTEST_DOMAINTEST_NAMETEST_VALUERESULT_IDRESULT_DATELOWER_BOUNDUPPER_BOUNDUNITSTEST_DESCRIPTIONMAX_RESULT_ID
FAILFrontiers Registryhictr_mrn_leading_zero042015-04-07 16:07:591PatientsVerify that we have at least one patient in the fact table who is a Frontiers Registry participant and has a 6-charater MRN after trimming off leading zeroes. This exposed a bug where we found the leading zeros were trimmed from MRNs in the IDX source but not in Clarity. See ticket 2451.4
fyitesttest_no_bound132015-04-07 16:07:55NothingJust show what a "no bound"" test looks like."3
oktesttest_success122015-04-07 16:07:5512NothingJust show what a successful test looks like.2
testtest_not_found_in_csv66612015-04-07 16:07:551

We added the following paver tasks:

> paver help | grep etl_test
  etl_tests_init                    - Initialize ETL tests
  etl_tests_report                  - Generate report for ETL tests
  load_etl_tests                    - Load ETL tests descriptions, thresholds, etc from .csv

comment:23 follow-up: Changed 3 years ago by ngraham

Matt/Dan reminded me that as mentioned in comment:21, we should make the "test_domain" field the top-level folder in HERON. I wonder what to do about things that cross domains? For example, "all facts use known concepts"? I suppose we could divide the concept list into where they fall in the hierarchy - so maybe we don't have things that cross domains after all.

comment:24 Changed 3 years ago by ngraham

Dan suggests adding a flag in the curated data file that means "don't run this test at all". Then, everywhere we have tests join against the relevant table and don't run the test if the flag is set.

comment:25 in reply to: ↑ 23 Changed 3 years ago by ngraham

Replying to ngraham:

Matt/Dan reminded me that as mentioned in comment:21, we should make the "test_domain" field the top-level folder in HERON.

Updated "Frontiers Registry" test to "Demographics" domain in [3e364cb3a5f7].

comment:26 Changed 3 years ago by ngraham

Brainstorming with Dan/Matt: Let's try to put a "commit" after every test so that during ETL we can monitor failures in real time. Done to tests already added in [c9776509e43f].

comment:27 Changed 3 years ago by dconnolly

I got some report formatting code working: 7915510b1d19 . It's not merged. The job grabs it from /tmp, currently.

But it's kinda purty. Hover over the test name for a special treat:

darn; the order by test_result desc, result_id isn't working with select *.

comment:28 Changed 3 years ago by ngraham

Last week I converted most of the medication transform 1/0 tests to the new exception report way of doing things - committed what I had in [1529968f3a16] and moving to other priorities (GPC:ticket:262, #3207).

comment:29 Changed 3 years ago by ngraham

  • Type changed from design-issue to enhancement

comment:30 Changed 3 years ago by ngraham

  • Blocking changed from 3207, 3394 to 3207, 3394, 3421

comment:31 Changed 3 years ago by ngraham

  • Resolution set to fixed
  • Status changed from accepted to closed

Current framework merged in [3dfdfbf5ad11] as part of clarity 2014 upgrade work. Exception report branch merged in [32a8ff6719c9].

Follow-on work to transition remaining tests to exception report framework in #3421.

comment:32 Changed 3 years ago by dconnolly

  • Blocking changed from 3207, 3394, 3421 to 3207, 3394, 3396, 3421
  • Resolution fixed deleted
  • Status changed from closed to reopened

Current UI for nightly results is a little klunky:

  1. run heron_etl_tiny_no_DBA
  2. run heron_tiny_exception_report

As part of validate and release (#3396), let's integrate this into heron_etl_log_summary_PROD

comment:33 Changed 3 years ago by dconnolly

  • Blocking changed from 3207, 3394, 3396, 3421 to 3207, 3396, 3421

already implicitly blocking ETL (#3394) via clarity 2014 (#3207)

comment:34 Changed 3 years ago by dconnolly

  • Resolution set to fixed
  • Status changed from reopened to closed

oops; didn't mean to re-open this after all (in comment:32)

comment:35 Changed 3 years ago by dconnolly

  • Blocking changed from 3207, 3396, 3421 to 3207, 3395, 3396, 3421

comment:36 Changed 3 years ago by dconnolly

  • Blocking changed from 3207, 3395, 3396, 3421 to 3207, 3395, 3421

comment:37 Changed 3 years ago by ngraham

  • Milestone changed from heron-cheyenne-bottoms-update to heron-quivira-update
  • Resolution fixed deleted
  • Status changed from closed to reopened

We put all this exception report stuff on the ID side rather than the DEID side so we can't run tests in DEID without digging ourselves deeper in the DEID->ID link issue (#482). A couple of options might be:

  1. Move all the exception report tables to the DEID side
    • Danger of sensitive information from tests leaking into a DEID database?
  2. Duplicate the the framework on ID and DIED
    • The report generation could union the two results tables
    • The unique test ID could be a concatenation of the sequence number and ID vs. DEID

I chatted with Matt and we lean towards option 2. Let's make the results table a view while we're at it.

comment:38 Changed 2 years ago by ngraham

  • Priority changed from major to minor

heron-weekly: we decided to demote.
Oops, not the ticket we were thinking of.

comment:39 Changed 2 years ago by ngraham

  • Milestone changed from heron-quivira-update to heron-jamestown-update
  • Priority changed from minor to major

comment:40 Changed 2 years ago by dconnolly

  • Milestone changed from heron-jamestown-update to dconnolly

Milestone renamed

comment:41 Changed 2 years ago by dconnolly

  • Milestone changed from dconnolly to heron-jamestown-update

Milestone renamed

comment:42 Changed 2 years ago by ngraham

  • Blocking changed from 3207, 3395, 3421 to 3207, 3395, 3421, 3536

comment:43 follow-up: Changed 2 years ago by ngraham

Design 2 from comment:37 implemented in [e3a135abcc59]. I plan to test once bmidev is available.

comment:44 Changed 2 years ago by ngraham

  • Owner changed from ngraham to mhoag
  • Status changed from reopened to assigned

Matt, could you review? See the resulting HTML report from the test build.

comment:45 in reply to: ↑ 43 Changed 2 years ago by mhoag

  • Owner changed from mhoag to ngraham

Replying to ngraham:

Design 2 from comment:37 implemented in [e3a135abcc59]. I plan to test once bmidev is available.

This looks like what we discussed in comment:37.

One nit on source:heron_load/etl_tests_report.sql#L24 is that the test_domain is concatenated with the realm and then renamed back to test_domain. While this has no functional problems it cause me mild semantic irritation. However, Nathan notes that not making this change is more concise, and I tend to agree.

I say this is go for merge.

comment:46 Changed 2 years ago by ngraham

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged in [28b824747376].

comment:47 Changed 2 years ago by dconnolly

  • Keywords ops added

not end-user visible

comment:48 Changed 2 years ago by ngraham

  • Keywords public-web added; ops removed

Not really ops - ETL related.

Note: See TracTickets for help on using tickets.