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

WIP: Add rough draft of SQL to be used in telemetry dashboard #3099

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akasper
Copy link
Collaborator

@akasper akasper commented Jan 6, 2025

Proof of concept

Summary

Proposed SQL DDL statements for the eCR Viewer dashboard telemetry

Acceptance Criteria

Review discussions and Notion documentation and validate that this schema is sensible.

Additional Information

This is highly subject to change. This code is proof-of-concept only. No need to merge the PR; simply presented for discussion.

Checklist

  • Discuss

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (0f6cfda) to head (43b40c4).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3099      +/-   ##
==========================================
+ Coverage   86.54%   88.54%   +1.99%     
==========================================
  Files         215       82     -133     
  Lines       13432     4181    -9251     
  Branches      666      704      +38     
==========================================
- Hits        11625     3702    -7923     
+ Misses       1798      457    -1341     
- Partials        9       22      +13     
Flag Coverage Δ
ecr-viewer 90.02% <ø> (+1.20%) ⬆️
fhir-converter ?
ingestion ?
message-parser ?
message-refiner ?
orchestration 85.67% <ø> (ø)
record-linkage ?
trigger-code-reference ?
validation ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 158 files with indirect coverage changes

run_id UUID PRIMARY KEY, -- Unique run ID
start_time TIMESTAMP NOT NULL, -- Start time of the run
end_time TIMESTAMP NOT NULL, -- End time of the run
is_complete BOOLEAN NOT NULL, -- Whether the run completed
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this derive-able from lack of end time? or does complete mean something different here (success perhaps)?

start_time TIMESTAMP NOT NULL, -- Start time of the run
end_time TIMESTAMP NOT NULL, -- End time of the run
is_complete BOOLEAN NOT NULL, -- Whether the run completed
duration INTERVAL GENERATED ALWAYS AS (end_time - start_time) STORED -- Duration of the run
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the thinking on storing this vs computing on the fly?

Comment on lines +14 to +15
set_id VARCHAR(255), -- ID for the set of eICRs
version_number VARCHAR(50), -- Version number of the eICR within the set
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this also or instead of include the ecr_id?

CREATE INDEX idx_runs_end_time ON runs (end_time);

CREATE TABLE activity_details (
run_id UUID PRIMARY KEY, -- Foreign key to runs table
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is the pk for the runs table also, what's the thinking behind having these be two separate tables?

CREATE INDEX idx_activity_reportable_condition ON activity_details (reportable_condition);

CREATE TABLE logging_details (
run_id UUID PRIMARY KEY, -- Foreign key to runs table
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this necessarily 1:1? could there be more than one error in the pipeline? or maybe its a mismatch of table name vs content - the content seems to be about errors, but logging can be much broader than that

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