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.

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

Add Flow run OTEL instrumentation #16010

Merged
merged 24 commits into from
Nov 14, 2024
Merged

Add Flow run OTEL instrumentation #16010

merged 24 commits into from
Nov 14, 2024

Conversation

collincchoy
Copy link
Contributor

@collincchoy collincchoy commented Nov 13, 2024

This PR adds OTEL instrumentation into the flow run engine so that if a user were to configure application-level telemetry, the flow run engine will now emit spans for flow runs that capture metadata about the flow run in span attributes and state changes as events on the span. Note: this PR does not include task run instrumentations but will extensibly nest spans (that can/will be emitted from tasks) transparently.

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 - heavily borrowing from opentelemetry.test.test_base import TestBase which many instrumentation libs in opentelemetry-python-contrib seem to use (i.e. httpx) but moved over in a more pytest-friendly interface here.

Closes CLOUD-564

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

@@ -560,6 +563,11 @@ class FlowRun(ObjectBaseModel):
description="A list of tags on the flow run",
examples=[["tag-1", "tag-2"]],
)
labels: KeyValueLabels = Field(
default_factory=dict,
description="Prefect Cloud: A dictionary of key-value labels. Values can be strings, numbers, or booleans.",
Copy link
Contributor Author

@collincchoy collincchoy Nov 13, 2024

Choose a reason for hiding this comment

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

labels are only available via cloud atm so adding them onto client schema here with a default dict to populate for non-cloud server responses so that they're available to be serialized as span attrs

@collincchoy collincchoy marked this pull request as draft November 13, 2024 22:43
Copy link

codspeed-hq bot commented Nov 13, 2024

CodSpeed Performance Report

Merging #16010 will not alter performance

Comparing flow-run-instrumentation (233a3ba) with main (86b80dc)

Summary

✅ 3 untouched benchmarks

@github-actions github-actions bot added the upstream dependency An upstream issue caused by a bug in one of our dependencies label Nov 14, 2024
@collincchoy collincchoy marked this pull request as ready for review November 14, 2024 17:31
@jeanluciano jeanluciano mentioned this pull request Nov 14, 2024
4 tasks
src/prefect/telemetry/test_utils.py Outdated Show resolved Hide resolved
src/prefect/flow_engine.py Outdated Show resolved Hide resolved
src/prefect/flow_engine.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

looks great!

@collincchoy collincchoy merged commit 377505b into main Nov 14, 2024
40 checks passed
@collincchoy collincchoy deleted the flow-run-instrumentation branch November 14, 2024 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream dependency An upstream issue caused by a bug in one of our dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants