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

CT-2049: Add CommandCompleted event #7180

Merged
merged 6 commits into from
Mar 22, 2023
Merged

CT-2049: Add CommandCompleted event #7180

merged 6 commits into from
Mar 22, 2023

Conversation

QMalcolm
Copy link
Contributor

@QMalcolm QMalcolm commented Mar 16, 2023 •
edited
Loading

resolves #6878

Description

CT-2049 outlines a desire to have an event that is output on completion of any and every command. This PR adds that functionality. Below is a demo of the functionality in a loom video (clicking will redirect you to loom)

Checklist

QMalcolm added 3 commits March 16, 2023 12:35
@QMalcolm
add protobuf message/class for new CommandCompleted event
ae93100 
For [CT-2049](#6878) we
concluded that we wanted a new event type, [CommandCompleted](#6878 (comment))
with [four (4) values](#6878 (comment)):
which command was run, whether the command succeeded, the timestamp
that the command finished, and how long the command took. This commit
adds the new event proto defition, the auto generated proto_types, and
the instantiatable even type.
@QMalcolm
begin emitting CommandCompleted event in the preflight decorator
7303278 
The [preflight decorator](https://github.com/dbt-labs/dbt-core/blob/4186f99b742b47e0e95aca4f604cc09e5c67a449/core/dbt/cli/requires.py#L19)
runs at the start of every CLI invocation. Thus is a perfect candidate
for emitting the CommandCompleted event. This is noted in the [dicussion
on CT-2049](#6878 (comment)).
@QMalcolm
add CommandCompleted event to event unit tests
0829ff7
@QMalcolm QMalcolm force-pushed the ct-2049-events-on-command-end branch from a4c05c3 to dd256b4 Compare March 16, 2023 19:35
@QMalcolm QMalcolm changed the title CT-2049: Add CommandComplted events CT-2049: Add CommandComplted event Mar 16, 2023
@dbt-labs dbt-labs deleted a comment from cla-bot bot Mar 16, 2023
@dbt-labs dbt-labs deleted a comment from github-actions bot Mar 16, 2023
@dbt-labs dbt-labs deleted a comment from cla-bot bot Mar 16, 2023
@QMalcolm QMalcolm force-pushed the ct-2049-events-on-command-end branch from dd256b4 to 10bce2c Compare March 16, 2023 19:39
@cla-bot cla-bot bot added the cla:yes label Mar 16, 2023
jtcohen6
jtcohen6 reviewed Mar 16, 2023
View reviewed changes
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

yo!! nice quick work - love to see an early draft PR :)

core/dbt/events/types.py Outdated Show resolved Hide resolved
.changes/unreleased/Under the Hood-20230316-123305.yaml Outdated Show resolved Hide resolved
QMalcolm added 3 commits March 16, 2023 13:12
@QMalcolm
Add: changelog entry
9860e11
@QMalcolm
fire CommandCompleted event reguardless of upstream exceptions
8f81ff8 
Previously, if `--fail-fast` was specified and an issue was run into
or an unhandled issue became an exception, the CommandCompleted event
would not get fired because at this point in the stack we'd be in
exception thrown handling mode. If an exception does reach this point,
we want to still fire the event and also continue to propogate the
exception. Hence the bare `raise` exists to reraise the caught exception
@QMalcolm
Update CommandCompleted event to be a Debug level event
92cec27 
We don't actually "always" need this event to be logged. Thus we've
updated it to `Debug` level. [Discussion Context](#7180 (comment))
@QMalcolm QMalcolm force-pushed the ct-2049-events-on-command-end branch from 10bce2c to 92cec27 Compare March 16, 2023 20:13
ChenyuLInx
ChenyuLInx reviewed Mar 16, 2023
View reviewed changes
core/dbt/cli/requires.py
# Bare except because we really do want to catch ALL exceptions,
# i.e. we want to fire this event in ALL cases.
except: # noqa
fire_event(
Copy link
Contributor

Choose a reason for hiding this comment

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

We used to fire another event when we run into issues, listed here, we actually have a issue tracking it here. @jtcohen6 maybe we can just resolve the other issue also in this PR(even though it is not scheduled), how you feel?

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally okay doing it separately to keep each PR smaller.

Copy link
Contributor Author

@QMalcolm QMalcolm Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that we should definitely work to fire that event again (and handle the fail fast exceptions better). I'm not opposed to taking on that work. However, I'd personally prefer to make it a separate PR to keep the PR process as smooth as possible.

Copy link
Contributor

@jtcohen6 jtcohen6 Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agree - let's do it in a separate PR! Even though that issue is not scheduled for the current sprint, we definitely want it before the v1.5 RC - we have it tracked in #7162

@QMalcolm QMalcolm marked this pull request as ready for review March 16, 2023 22:33
@QMalcolm QMalcolm requested review from a team as code owners March 16, 2023 22:33
gshank
gshank approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@gshank gshank 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.

stu-k
stu-k reviewed Mar 21, 2023
View reviewed changes
Copy link
Contributor

@stu-k stu-k left a comment

Choose a reason for hiding this comment

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

I know it's already a little bloated, but it might be nice to move your work out of the preflight decorator as that's meant to contain setup type initialization, not managing the resulting data.

@QMalcolm
Copy link
Contributor Author

I know it's already a little bloated, but it might be nice to move your work out of the preflight decorator as that's meant to contain setup type initialization, not managing the resulting data.

Hey @stu-k! I'm happy to move it out of the preflight decorator 😄 The only question is, "where?" 🤔 It's worth mentioning that the "where" was previously discussed and the conclusion at the time was that the preflight decorator might be the best place. Looking at the existing decorators, I don't believe there is another good candidate currently. Thus the alternative would be to create a new decorator simply for the handling of this functionality, and using it with every command. Let me know what you think 🙂

@QMalcolm QMalcolm changed the title CT-2049: Add CommandComplted event CT-2049: Add CommandCompleted event Mar 21, 2023
ChenyuLInx
ChenyuLInx approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

LGTM!!! Thanks for also have a loom videos showing how this works under all of the failure modes!

@ChenyuLInx
Copy link
Contributor

Also agreed on let's leave the move it out of the preflight decorator out of this PR. Since it probably apply to both this issue and the exit code issue.

@stu-k
Copy link
Contributor

stu-k commented Mar 21, 2023

@QMalcolm We can follow up on the moving into another place in another ticket!

@QMalcolm QMalcolm merged commit 9a7305d into main Mar 22, 2023
@QMalcolm QMalcolm deleted the ct-2049-events-on-command-end branch March 22, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stu-k stu-k stu-k left review comments

@jtcohen6 jtcohen6 jtcohen6 left review comments

@ChenyuLInx ChenyuLInx ChenyuLInx approved these changes

@gshank gshank gshank approved these changes

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

@Fleid Fleid Awaiting requested review from Fleid Fleid was automatically assigned from dbt-labs/core-group

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-2049] Fire an event at the end of every command
5 participants
@QMalcolm @ChenyuLInx @stu-k @gshank @jtcohen6