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

Add span uri #415

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Yicheng-Lu-llll
Copy link
Member

@Yicheng-Lu-llll Yicheng-Lu-llll commented Jun 8, 2023

TL;DR

This is part of Flytekit Metrics Exploration

This PR(also highlighted in green):

  • adds span uri field to NodeExecutionEvent and NodeExecutionClosure.
  • adds a new endpoint GetFlyteKitMetrics.
image

Flytekit Metrics Exploration includes:

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

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

@welcome
Copy link

welcome bot commented Jun 8, 2023

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Signed-off-by: Yicheng-Lu-llll <[email protected]>
@Yicheng-Lu-llll Yicheng-Lu-llll changed the title [WIP] Add span uri Add span uri Jun 14, 2023
@Yicheng-Lu-llll Yicheng-Lu-llll marked this pull request as ready for review June 14, 2023 19:25
@@ -171,6 +171,8 @@ message NodeExecutionClosure {
// dynamic_job_spec_uri is the location of the DynamicJobSpec proto message for a DynamicWorkflow. This is required
// to correctly recover partially completed executions where the subworkflow has already been compiled.
string dynamic_job_spec_uri = 12;

string span_uri = 13;
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment here

@@ -110,6 +110,8 @@ message NodeExecutionEvent {
// literal inputs are initially copied. The event however will not be sent until after the copy completes.
// Extracting both of these timestamps facilitates a more accurate portrayal of the evaluation time-series.
google.protobuf.Timestamp reported_at = 21;

string span_uri = 22;
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -636,4 +636,14 @@ service AdminService {
// description: "Retrieve metrics from an existing workflow execution."
// };
};

// Fetches FlyteKit metrics for a :ref:`ref_flyteidl.admin.NodeExecution`.
rpc GetFlyteKitMetrics (flyteidl.admin.NodeExecutionGetRequest) returns (flyteidl.admin.WorkflowExecutionGetMetricsResponse) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename it to GetTaskMetrics? cc @hamersaw

@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #415 (0d7bea7) into master (e506206) will increase coverage by 2.55%.
The diff coverage is n/a.

❗ Current head 0d7bea7 differs from pull request most recent head 1cfa4ee. Consider uploading reports for the commit 1cfa4ee to get more accurate results

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
+ Coverage   75.92%   78.48%   +2.55%     
==========================================
  Files          18       18              
  Lines        1458     1250     -208     
==========================================
- Hits         1107      981     -126     
+ Misses        294      212      -82     
  Partials       57       57              
Flag Coverage Δ
unittests ?

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

see 18 files with indirect coverage changes

Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants