Add AsyncDDOGTracer #1247

rwhitten577 · 2024-12-02T18:43:07Z

Adds AsyncDDOGTracer, which has all the same functionality of DDOGTracer but inherits from the base async lifecycle classes to make it work with the AsyncDriver. Fixes #1236.

Changes

Add AsyncDDOGTracer
Refactor DDOGTracer functionality into private _DDOGTracerImpl, which allows sharing logic between the sync & async versions
Added check for current trace context so Hamilton DAG run gets properly nested under any open parent spans. This fixed the issue I had where my FastAPI route and Hamilton run were 2 independent traces in DDOG. It will be a no-op if there is no current trace.
Update docs to add AsyncDDOGTracer reference

How I tested this

Tested 1.83.3 to verify behavior of all nodes showing executed quickly at the beginning:
Tested new implementation of standard DDOGTracer to ensure it works as expected (notice Hamilton is now nested under the FastAPI parent span):
Tested new async implementation:

Notes

Checklist

PR has an informative and human-readable title (this will be pulled into the release notes)
Changes are limited to a single goal (no scope creep)
Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
Any change in functionality is tested
New functions are documented (with a description, list of inputs, and expected output)
Placeholder code is flagged / future TODOs are captured in comments
Project documentation has been updated if adding/changing functionality.

skrawcz · 2024-12-03T07:16:41Z

Exciting! Let us know when to review (should we wait for it for this week's release?). Otherwise we'll rely on you for testing that it works for real ;)

ellipsis-dev

❌ Changes requested. Reviewed everything up to 580bb56 in 43 seconds

More details

Looked at 338 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. hamilton/plugins/h_ddog.py:391

Draft comment:
The docstring for AsyncDDOGTracer is repeated from DDOGTracer. Consider refactoring to avoid repetition and adhere to the DRY principle.
Reason this comment was not posted:
Marked as duplicate.

Workflow ID: wflow_3sEbPU4BnSyc1fM2

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2024-12-03T15:01:28Z

hamilton/plugins/h_ddog.py

+    ):
+        """Runs after graph execution. Garbage collects + finishes the root span.
+
+        :param error: Error the graph raised when running, if any


The run_after_task_execution method is not utilized in AsyncDDOGTracer. Consider implementing this method for async task execution as well.

This is rightly pointed out, but not sure if I need it: is async mode & dynamic/task mode compatible? I assume "task" here refers to the parallelize/collect flow.

Yes, this is specifically not supported (right now). I think this is good for now -- add a note about it.

hamilton/plugins/h_ddog.py

rwhitten577 · 2024-12-03T15:41:54Z

Exciting! Let us know when to review (should we wait for it for this week's release?). Otherwise we'll rely on you for testing that it works for real ;)

Thanks @skrawcz, ready to review. Tested out both sync and async versions, and added some screenshots here in the description

rwhitten577 · 2024-12-03T15:43:00Z

hamilton/plugins/h_ddog.py

@@ -130,7 +106,10 @@ def run_before_graph_execution(self, *, run_id: str, **future_kwargs: Any):
        :param run_id: ID of the run
        :param future_kwargs: reserved for future keyword arguments/backwards compatibility.
        """
-        span = tracer.start_span(name=self.root_name, activate=True, service=self.service)
+        current_context = tracer.current_trace_context()


This returns None if there's no active trace, so will be a safe no-op if there isn't one. child_of in start_span is None by default, so we're just assigning the parent if it exists.

Worth adding this as a comment

Good idea — added!

elijahbenizzy

Looks really clean! Nice work. Only comments are about comments int he code -- a little bit of design choice can make it a bit easier to maintain.

Otherwise if you add some comments and push another one I'll merge + release shortly.

elijahbenizzy · 2024-12-03T18:29:31Z

hamilton/plugins/h_ddog.py

-    To use this, you'll want to run `pip install sf-hamilton[ddog]` (or `pip install "sf-hamilton[ddog]"` if using zsh)
-    """
-
+class _DDOGTracerImpl:


Add a docstring for the reasoning of having a composition-based class (rather than inheritance) -- e.g. sync versus async. Good choice, IMO, but worth clarifying in the code.

Thanks, added!

elijahbenizzy · 2024-12-03T18:29:57Z

hamilton/plugins/h_ddog.py

@@ -130,7 +106,10 @@ def run_before_graph_execution(self, *, run_id: str, **future_kwargs: Any):
        :param run_id: ID of the run
        :param future_kwargs: reserved for future keyword arguments/backwards compatibility.
        """
-        span = tracer.start_span(name=self.root_name, activate=True, service=self.service)
+        current_context = tracer.current_trace_context()


Worth adding this as a comment

elijahbenizzy · 2024-12-03T23:33:25Z

hamilton/plugins/h_ddog.py

+    ):
+        """Runs after graph execution. Garbage collects + finishes the root span.
+
+        :param error: Error the graph raised when running, if any


Yes, this is specifically not supported (right now). I think this is good for now -- add a note about it.

rwhitten577 added 2 commits December 2, 2024 13:23

Add AsyncDDOGTracer

1b4f69b

Refactor DDOG tracers to use common impl class

37f7d65

Fix ref to _sanitize_tags method

580bb56

rwhitten577 marked this pull request as ready for review December 3, 2024 15:00

ellipsis-dev bot reviewed Dec 3, 2024

View reviewed changes

rwhitten577 commented Dec 3, 2024

View reviewed changes

rwhitten577 mentioned this pull request Dec 3, 2024

Update DDOGTracer to work with AsyncDriver #1236

Closed

skrawcz requested a review from elijahbenizzy December 3, 2024 17:36

elijahbenizzy approved these changes Dec 3, 2024

View reviewed changes

Add DDOGTracer comments to explain impl class & assigning parent context

b1815e4

rwhitten577 requested a review from elijahbenizzy December 4, 2024 15:21

elijahbenizzy approved these changes Dec 4, 2024

View reviewed changes

elijahbenizzy merged commit 69b1a62 into DAGWorks-Inc:main Dec 4, 2024
24 checks passed

rwhitten577 deleted the feature/add-async-ddog-tracer branch December 4, 2024 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AsyncDDOGTracer #1247

Add AsyncDDOGTracer #1247

rwhitten577 commented Dec 2, 2024 •

edited

Loading

skrawcz commented Dec 3, 2024

ellipsis-dev bot left a comment

ellipsis-dev bot Dec 3, 2024

rwhitten577 Dec 3, 2024

elijahbenizzy Dec 3, 2024

rwhitten577 commented Dec 3, 2024

rwhitten577 Dec 3, 2024

elijahbenizzy Dec 3, 2024

rwhitten577 Dec 4, 2024

elijahbenizzy left a comment

elijahbenizzy Dec 3, 2024

rwhitten577 Dec 4, 2024

elijahbenizzy Dec 3, 2024

elijahbenizzy Dec 3, 2024

Add AsyncDDOGTracer #1247

Add AsyncDDOGTracer #1247

Conversation

rwhitten577 commented Dec 2, 2024 • edited Loading

Changes

How I tested this

Notes

Checklist

skrawcz commented Dec 3, 2024

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwhitten577 commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elijahbenizzy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwhitten577 commented Dec 2, 2024 •

edited

Loading