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.

Sign up for GitHub

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

Jump to bottom

Add Window Attribute for Metrics #5722

Merged
merged 9 commits into from
Sep 7, 2022
Merged

Add Window Attribute for Metrics #5722

merged 9 commits into from
Sep 7, 2022

Conversation

callum-mcdata
Copy link
Contributor

@callum-mcdata callum-mcdata commented Aug 26, 2022 •
edited
Loading

resolves #5721

Description

Adding the attribute window to the metric definition as an optional string. No tests have been added yet as this is merely a draft.

Produced Manifest

I tested this out in a development project and was able to confirm that "window": "14 days" was correctly represented in the single metric where it was defined. For all others "window": "" was present.

Checklist

@cla-bot cla-bot bot added the cla:yes label Aug 26, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@leahwicz
Copy link
Contributor

@callum-mcdata once you are ready for an Engineering team member to review (test passing, name solidified), feel free to add the ready_for_review label to the PR so we know to take a look

@callum-mcdata callum-mcdata changed the title Add Lookback Attribute for Metrics Add Window Attribute for Metrics Aug 31, 2022
@callum-mcdata callum-mcdata marked this pull request as ready for review August 31, 2022 19:34
@callum-mcdata callum-mcdata requested review from a team as code owners August 31, 2022 19:34
@callum-mcdata callum-mcdata added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Aug 31, 2022
@callum-mcdata
Copy link
Contributor Author

callum-mcdata commented Aug 31, 2022 •
edited
Loading

Hey @leahwicz this PR is ready for review! I updated the tests to ensure that they pass (ie adding window into the pre-created dicts that we compare against). If the PR gets the 🟢 from the core team I'll make the corresponding updates to the docs repo before we merge!

@leahwicz leahwicz requested a review from emmyoop August 31, 2022 19:42
@leahwicz
Copy link
Contributor

Thanks @callum-mcdata! I just added @emmyoop as a reviewer since she is on support rotation right now

emmyoop
emmyoop reviewed Sep 1, 2022
View reviewed changes
Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

Just a nit and a few questions.

.changes/unreleased/Features-20220831-121319.yaml Outdated Show resolved Hide resolved
core/dbt/contracts/graph/unparsed.py Show resolved Hide resolved
core/dbt/contracts/graph/unparsed.py Outdated Show resolved Hide resolved
@emmyoop emmyoop removed the request for review from ChenyuLInx September 1, 2022 15:47
@callum-mcdata
Copy link
Contributor Author

@emmyoop made the changes that you mentioned in above resolved conversations!

emmyoop
emmyoop approved these changes Sep 6, 2022
View reviewed changes
Copy link
Member

@emmyoop emmyoop 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!

@callum-mcdata
Copy link
Contributor Author

@gshank could I get approval to merge in this PR? Looks like I'm not an authorized user to merge into main

@leahwicz leahwicz merged commit e3452b9 into main Sep 7, 2022
@leahwicz leahwicz deleted the adding_metric_lookback branch September 7, 2022 14:45
@callum-mcdata callum-mcdata mentioned this pull request Sep 8, 2022
Merged
6 tasks
josephberni pushed a commit to Gousto/dbt-core that referenced this pull request Sep 16, 2022
@callum-mcdata @josephberni
Add Window Attribute for Metrics (dbt-labs#5722)
43d0da6 
* file changes

* changing to window

* adding test

* adding changie for feature

* fixing commits

* fixing tests

* adding timestamp

* fixing graph unparsed

* changing default value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmyoop emmyoop emmyoop approved these changes

@gshank gshank Awaiting requested review from gshank gshank is a code owner automatically assigned from dbt-labs/core-team

Assignees
No one assigned
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-1091] [Feature] Add Lookback Attribute for Metrics
4 participants
@callum-mcdata @leahwicz @emmyoop @jtcohen6