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

OTEL flow run context propagation with Labels #16122

Merged

Conversation

jeanluciano
Copy link
Contributor

@jeanluciano jeanluciano commented Nov 26, 2024

This PR adds OTEL context propagation when __OTEL_TRACEPARENTS are found in a parent flow run. It also adds a way to update flow run labels with update_flow_run_labels. This is so that if no __OTEL_TRACEPARENT is found we can update the label with the newly created span's context.

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.

Copy link

codspeed-hq bot commented Nov 26, 2024

CodSpeed Performance Report

Merging #16122 will not alter performance

Comparing jean/cloud-734-move-update_flow_run_labels-to-standard-client (653eb55) with main (8fd92fe)

Summary

✅ 3 untouched benchmarks

src/prefect/server/api/flow_runs.py Fixed Show fixed Hide fixed
src/prefect/server/models/flow_runs.py Fixed Show fixed Hide fixed
src/prefect/server/models/flow_runs.py Fixed Show fixed Hide fixed
src/prefect/server/models/flow_runs.py Fixed Show fixed Hide fixed
src/prefect/server/models/flow_runs.py Fixed Show fixed Hide fixed
@bunchesofdonald bunchesofdonald force-pushed the jean/cloud-734-move-update_flow_run_labels-to-standard-client branch from 4a563c1 to d435acd Compare December 2, 2024 16:33
@bunchesofdonald bunchesofdonald force-pushed the jean/cloud-734-move-update_flow_run_labels-to-standard-client branch from d435acd to 6bc104a Compare December 2, 2024 17:02
@jeanluciano jeanluciano marked this pull request as ready for review December 4, 2024 15:59
@jeanluciano jeanluciano marked this pull request as draft December 4, 2024 17:01
…/cloud-734-move-update_flow_run_labels-to-standard-client
@jeanluciano jeanluciano marked this pull request as ready for review December 4, 2024 17:56
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.

a couple comments to start

src/prefect/client/schemas/actions.py Outdated Show resolved Hide resolved
src/prefect/flow_engine.py Outdated Show resolved Hide resolved
src/prefect/server/models/flow_runs.py Outdated Show resolved Hide resolved
tests/test_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.

looking pretty good! one more comment

@jeanluciano jeanluciano requested a review from zzstoatzz December 5, 2024 19:50
src/prefect/client/orchestration.py Outdated Show resolved Hide resolved
src/prefect/client/orchestration.py Outdated Show resolved Hide resolved
src/prefect/flow_engine.py Outdated Show resolved Hide resolved
src/prefect/server/models/flow_runs.py Outdated Show resolved Hide resolved
@@ -2094,3 +2094,84 @@ def a_slow_flow():
"exception.escaped": "False",
},
)

async def test_flow_run_propagates_otel_traceparent_to_subflow(
Copy link
Member

Choose a reason for hiding this comment

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

Might be worthwhile to parameterize this with a sync and async flow now that the engine is split.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have it done in an upstream PR #16233

assert child_span.context and parent_span.context
assert child_span.context.trace_id == parent_span.context.trace_id

async def test_flow_run_creates_and_stores_otel_traceparent(
Copy link
Member

Choose a reason for hiding this comment

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

Might be worthwhile to parameterize this with a sync and async flow now that the engine is split.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

Two small requests on the orchestration client changes, but otherwise LGTM!

Comment on lines 3465 to 3476
async def update_flow_run_labels(
self, flow_run_id: UUID, labels: KeyValueLabelsField
) -> httpx.Response:
"""
Updates the labels of a flow run.
"""

response = await self._client.patch(
f"/flow_runs/{flow_run_id}/labels", json=labels
)
response.raise_for_status()
return response
Copy link
Member

Choose a reason for hiding this comment

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

Let's not return the response from here since it looks like it isn't needed.

Suggested change
async def update_flow_run_labels(
self, flow_run_id: UUID, labels: KeyValueLabelsField
) -> httpx.Response:
"""
Updates the labels of a flow run.
"""
response = await self._client.patch(
f"/flow_runs/{flow_run_id}/labels", json=labels
)
response.raise_for_status()
return response
async def update_flow_run_labels(
self, flow_run_id: UUID, labels: KeyValueLabelsField
) -> None:
"""
Updates the labels of a flow run.
"""
response = await self._client.patch(
f"/flow_runs/{flow_run_id}/labels", json=labels
)
response.raise_for_status()

Comment on lines 4390 to 4401
def update_flow_run_labels(
self, flow_run_id: UUID, labels: KeyValueLabelsField
) -> httpx.Response:
"""
Updates the labels of a flow run.
"""
response = self._client.patch(
f"/flow_runs/{flow_run_id}/labels",
json=labels,
)
response.raise_for_status()
return response
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Suggested change
def update_flow_run_labels(
self, flow_run_id: UUID, labels: KeyValueLabelsField
) -> httpx.Response:
"""
Updates the labels of a flow run.
"""
response = self._client.patch(
f"/flow_runs/{flow_run_id}/labels",
json=labels,
)
response.raise_for_status()
return response
def update_flow_run_labels(
self, flow_run_id: UUID, labels: KeyValueLabelsField
) -> None:
"""
Updates the labels of a flow run.
"""
response = self._client.patch(
f"/flow_runs/{flow_run_id}/labels",
json=labels,
)
response.raise_for_status()

@jeanluciano jeanluciano merged commit 8b740d4 into main Dec 9, 2024
32 of 36 checks passed
@jeanluciano jeanluciano deleted the jean/cloud-734-move-update_flow_run_labels-to-standard-client branch December 9, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants