DDOG Integration #644

elijahbenizzy · 2024-01-16T04:53:07Z

Iterating -- plan of attack/thoughts:

Opted to store a cache myself and prune rather than using context as I want to leave the door open for more clever uses of this. I just link parents and store a dict
Should be easy to get tasks working, just need to expose the API
Should also be possible to add causal links between spans as well. TBD on how to use the DDOG API exactly.
Add all node tags to DDOG cause why not 🤷
do OpenTel (out of scope for this, that's next)

What this looks like:

Changes

DDOG
An extra (easter-egg) hook
Improvements to running hooks in the case of exceptions

How I tested this

ran ddog multiple times locally under different contexts
tests passed for existing ones (what I was fixing for was not worth testing for TBH, parity with current behavior is good)

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.

We were missing this -- this adds it in.

This is a bit of a joke -- it just injects latency into the DAG. Purely for testing, but perhaps someone can do something fun with it.

This is functionally the same as before, it just hits a weird edge case in which a failure to deliver the hook would be called twice, as it would fail when running it and fail when reacting to the failure (in the catch). We need to do this for graph/node.

elijahbenizzy · 2024-01-17T05:18:31Z

hamilton/lifecycle/base.py

        for adapter in self.adapters:
            for cls in inspect.getmro(adapter.__class__):
                sync_hook = getattr(cls, SYNC_HOOK, None)
                if sync_hook is not None:
-                    sync_hooks[sync_hook].add(adapter)
+                    if adapter not in sync_hooks[sync_hook]:
+                        sync_hooks[sync_hook].append(adapter)


OK, this is not ironed into the contract, and we'll likely want to switch around order, but I think for the future we need to think about this a bit -- my instinct says to do pre_ in order and post_ in reverse order, but that's complicated and ambiguous to implement.

yes we should do it that way ideally

elijahbenizzy · 2024-01-17T05:18:56Z

hamilton/plugins/h_ddog.py

+from hamilton import lifecycle
+
+
+class DDOGTracer(


Could add testing but I don't think its worth it, I'll just fix forward and if its a pain add testing

elijahbenizzy · 2024-01-17T05:19:09Z

hamilton/lifecycle/default.py

+    time.sleep(sleep_time)
+
+
+class SlowDownYouMoveTooFast(NodeExecutionHook):


hamilton/plugins/h_ddog.py

skrawcz · 2024-01-17T06:53:32Z

hamilton/plugins/h_ddog.py

-    ) -> Tuple[Optional[str], ...]:
-        return run_id, task_id, node_id
+    def _serialize_span_dict(span_dict: Dict[str, Span]):
+        """Serializes to a readable format. We're not propogating links, but that's fine (for now).


what do you mean link?

Span links -- see notes elsewhere

This adds basic tracing support for the datadog plugin. We elect to manage the state of spans/traces ourselves, and there is a decent amount of complexity around serialization. It works on Ray, Dask, Multithreading, and Synchronous mode, as well as vanilla Hamilton (non-task-based). It can optionally store links, although datadogs functionality is pretty suboptimal and has large UI query times, so that is turned off by default.

This ensures that it is only run once, and cleans up the code.

This ensures we don't accidentally run it twice if it fails, and cleans up the code.

This makes it slightly easier to filter by node name, across, say, different tasks. We namespace it under hamilton to ensure it doesn't clash.

We were doing the wrong ones for required_dependencies. This fixes that, and also uses input_types (rather than dependencies) for the required/optional dependencies, which is guarenteed to exist when the node is created (as opposed to dependencies which is set when we create the DAG.

DDOG: POC/WIP

23aa293

DAGWorks-Inc deleted a comment from sweep-ai bot Jan 16, 2024

elijahbenizzy force-pushed the ddog-tracing branch from 0b8dc90 to baeb0d8 Compare January 17, 2024 01:38

elijahbenizzy added 3 commits January 16, 2024 20:51

Adds TaskExecutionHook

62a9b2b

We were missing this -- this adds it in.

Adds SlowDownYouMoveTooFastHook

50e1abf

This is a bit of a joke -- it just injects latency into the DAG. Purely for testing, but perhaps someone can do something fun with it.

elijahbenizzy force-pushed the ddog-tracing branch from baeb0d8 to c09bdbf Compare January 17, 2024 04:59

elijahbenizzy changed the title ~~DDOG: POC/WIP~~ DDOG Integration Jan 17, 2024

elijahbenizzy force-pushed the ddog-tracing branch from 2de00ca to eef0e60 Compare January 17, 2024 05:16

elijahbenizzy commented Jan 17, 2024

View reviewed changes

hamilton/lifecycle/default.py

time.sleep(sleep_time)

class SlowDownYouMoveTooFast(NodeExecutionHook):

Copy link

Collaborator Author

elijahbenizzy Jan 17, 2024

Choose a reason for hiding this comment

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

Easter egg

elijahbenizzy requested a review from skrawcz January 17, 2024 05:21

elijahbenizzy force-pushed the ddog-tracing branch from eef0e60 to 13366f8 Compare January 17, 2024 05:30

skrawcz approved these changes Jan 17, 2024

View reviewed changes

elijahbenizzy force-pushed the ddog-tracing branch 4 times, most recently from 68343d0 to 319234f Compare January 17, 2024 18:09

elijahbenizzy added 5 commits January 17, 2024 10:11

Moves post_node_execute hook to finally block

b561433

This ensures that it is only run once, and cleans up the code.

Moves post_graph_execute hook to finally block

6fe5e0d

This ensures we don't accidentally run it twice if it fails, and cleans up the code.

Adds hamilton.node_name as tag to datadog node

cd06d9e

This makes it slightly easier to filter by node name, across, say, different tasks. We namespace it under hamilton to ensure it doesn't clash.

elijahbenizzy force-pushed the ddog-tracing branch from 319234f to 4120268 Compare January 17, 2024 18:11

elijahbenizzy merged commit c642615 into main Jan 17, 2024
22 checks passed

elijahbenizzy deleted the ddog-tracing branch January 17, 2024 18:21

elijahbenizzy mentioned this pull request Jan 18, 2024

Lifecycle hooks don't work with ctrl-c #619

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DDOG Integration #644

DDOG Integration #644

elijahbenizzy commented Jan 16, 2024 •

edited

Loading

elijahbenizzy Jan 17, 2024

skrawcz Jan 17, 2024

elijahbenizzy Jan 17, 2024

elijahbenizzy Jan 17, 2024

skrawcz Jan 17, 2024

elijahbenizzy Jan 17, 2024

		time.sleep(sleep_time)


		class SlowDownYouMoveTooFast(NodeExecutionHook):

DDOG Integration #644

DDOG Integration #644

Conversation

elijahbenizzy commented Jan 16, 2024 • edited Loading

Changes

How I tested this

Notes

Checklist

elijahbenizzy Jan 17, 2024

Choose a reason for hiding this comment

skrawcz Jan 17, 2024

Choose a reason for hiding this comment

elijahbenizzy Jan 17, 2024

Choose a reason for hiding this comment

elijahbenizzy Jan 17, 2024

Choose a reason for hiding this comment

skrawcz Jan 17, 2024

Choose a reason for hiding this comment

elijahbenizzy Jan 17, 2024

Choose a reason for hiding this comment

elijahbenizzy commented Jan 16, 2024 •

edited

Loading