-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Instrument task runs #15955
base: main
Are you sure you want to change the base?
Instrument task runs #15955
Conversation
CodSpeed Performance ReportMerging #15955 will not alter performanceComparing Summary
|
…/cloud-565-task-run-instrumentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few changes that stood out to me; still need to review more closely
…/cloud-565-task-run-instrumentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this lgtm! cc @desertaxle for a second look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but I think we can clean up the code a little by moving most of the new otel functionality to the base class and using it in the separate sync and async engines. Also, we'll want some coverage on the SyncTaskRunEngine
.
…hub.com/PrefectHQ/prefect into jean/cloud-565-task-run-instrumentation
…/cloud-565-task-run-instrumentation
…hub.com/PrefectHQ/prefect into jean/cloud-565-task-run-instrumentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking pretty good! a couple comments
def get_labels_from_context(context: Optional[FlowRunContext]) -> Dict[str, Any]: | ||
if context is None: | ||
return {} | ||
return context.flow_run.labels | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def get_labels_from_context(context: Optional[FlowRunContext]) -> Dict[str, Any]: | |
if context is None: | |
return {} | |
return context.flow_run.labels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is now unused
parameters: Dict[str, Any] = {}, | ||
labels: Dict[str, Any] = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these mutable defaults should be optionals that default to None
Adds OTEL instrumentation into the task engine so that if a user were to configure application-level telemetry, the task run engine will now emit spans for runs that capture metadata about the task run in span attributes and state changes as events on the span.
What impact will this have if I do not have telemetry configured?
https://opentelemetry.io/docs/concepts/instrumentation/libraries/#performance
Performance
OpenTelemetry API is no-op and very performant when there is no SDK in the application
This PR also sets up some testing infrastructure for testing otel instrumentation implemented by @collincchoy in #16010
Checklist
<link to issue>
"mint.json
.