Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Tracking reasons time-series #540

Merged
merged 3 commits into from
Mar 22, 2023
Merged

Tracking reasons time-series #540

merged 3 commits into from
Mar 22, 2023

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Mar 14, 2023

TL;DR

This PR populates the new Reasons field on the TaskExecutionClosure to track a reason time-series. This will be very useful in improving visualizations by tracking the reason for task phase transitions and updates.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

^^^

Tracking Issue

flyteorg/flyte#3501

Follow-up issue

NA

Signed-off-by: Daniel Rammer <daniel@union.ai>
@@ -151,6 +151,15 @@ func CreateTaskExecutionModel(ctx context.Context, input CreateTaskExecutionMode
EventVersion: input.Request.Event.EventVersion,
}

if len(input.Request.Event.Reason) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this right? should it not check for len(closure.Reasons) <= 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this is run in CreateTaskExecutionModel so closure.Reasons will always be 0 because we just created the closure. This just says - don't add a reason to the time-series if the reason is an empty string.

Signed-off-by: Daniel Rammer <daniel@union.ai>
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw hamersaw marked this pull request as ready for review March 21, 2023 07:25
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #540 (42bafaa) into master (dea5b2a) will increase coverage by 1.54%.
The diff coverage is 100.00%.

❗ Current head 42bafaa differs from pull request most recent head 542a926. Consider uploading reports for the commit 542a926 to get more accurate results

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
+ Coverage   60.18%   61.72%   +1.54%     
==========================================
  Files         169      169              
  Lines       15106    12437    -2669     
==========================================
- Hits         9091     7677    -1414     
+ Misses       5214     3959    -1255     
  Partials      801      801              
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
pkg/repositories/transformers/task_execution.go 75.98% <100.00%> (+3.53%) ⬆️

... and 154 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hamersaw hamersaw merged commit 83e8694 into master Mar 22, 2023
@hamersaw hamersaw deleted the feature/reason-time-series branch March 22, 2023 13:33
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* tracking reasons time-series

Signed-off-by: Daniel Rammer <daniel@union.ai>

* bumped flyteidl dep and added comment

Signed-off-by: Daniel Rammer <daniel@union.ai>

---------

Signed-off-by: Daniel Rammer <daniel@union.ai>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants