Page MenuHomePhabricator

[RFC] Event Trailers
AbandonedPublic

Authored by sashank on Mar 18 2020, 12:57 AM.

Details

Summary

This diff introduces the idea of event trailers, which can be configured on the instance. A single instance can configure multiple event trailers, since users will likely need to ship the logs to multiple destinations - for example Datadog + our hosted product.

An event trailer, inspired by the run laucher, is as thin as possible and only implements a single method: handle_new_event.

Example configuration:

event_trailers:
  - module: dagster_datadog.event_trailer
    class: DatadogEventTrailer
    config:
      api_key: "1235"
      app_key: "1234"

Here is a work in progress DatadogEventTrailer, which for now sends a HTTP post request to RequestBin instead of sending a request to Datadog. You can view the output here: https://requestbin.com/r/en2vcxlva94f9/1ZHOXfm21Smp5wMLEC2FQJNRujx

Test Plan

Todo

Diff Detail

Repository
R1 dagster
Branch
datadog-logger
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sashank created this revision.Mar 18 2020, 12:57 AM
sashank edited the summary of this revision. (Show Details)Mar 18 2020, 1:01 AM
sashank updated this revision to Diff 10648.Mar 18 2020, 1:02 AM

Remove logger from this diff

Interesting approach! My intuition was that we would add these things via ModeDefinition not the instance. Putting this on the instance makes it difficult to distinguish test runs from prod runs from etc. Guess this could be solved by having it only set on the prod deployment instance config, but that makes testing the tailer a bit taxing. Think its worth thinking through the difference in these two approaches.

"Trailer" is an interesting name - "Listener" may be a suitable more common name.

It might be nice to have some light API on this class - stuff like on_pipeline_failure to make it easy to write one that just pings something when stuff fails.

python_modules/dagster/dagster/core/instance/__init__.py
458–459

the real star of the show

Add these things via ModeDefinition not the instance

This makes sense, since it would be in line with how we work with execution engines, loggers, etc. I can put up another RFC examining this approach. In the limit, it seems like it would be best to be able to configure global settings via the instance, but be able to override via mode/config.

My only concern is that if someone wanted to ship their logs using an event listener, they would have to add the listener to _every_ mode definition they have in their repository, rather than just once for their entire instance. I'm not sure if users will want to selectively decide on a per-pipeline basis whether they want to ship their logs, or would want every log shipped. cc @nate

Makes testing the tailer a bit taxing

Wouldn't this be tested just like a run launcher or scheduler would?

Wouldn't this be tested just like a run launcher or scheduler would?

ya - like testing the tricky bits of those that don't run locally

This makes sense, since it would be in line with how we work with execution engines, loggers,

I think there might be a way to do this by injecting a logger of a new class that knows how to reconstitute the DagsterEvent s from the logs

sashank updated this revision to Diff 10719.EditedMar 19 2020, 7:56 PM

Event Trailer -> Event Listener
Send events to datadog instead of request bin

sashank added a comment.EditedMar 19 2020, 7:58 PM

That was the first approach I was starting to take here in loggers.py: https://dagster.phacility.com/D2274?vs=on&id=10647#change-WG3qhAXuWmth, but thought it was a bit gross to re-construct dagster events from logs when we could just read them directly.

alangenfeld added inline comments.Mar 19 2020, 8:03 PM
python_modules/libraries/dagster-datadog/dagster_datadog/loggers.py
16–18 ↗(On Diff #10647)

you can get the dagster event via record.dagster_meta.get('dagster_event')

a bit gross to re-construct dagster events from logs when we could just read them directly

you don't need to rebuild them - but you are not wrong that this approach feels a little gross. The benefits of this approach are delivering the desired user capability with the fewest new systems/features

sashank added inline comments.Mar 19 2020, 8:12 PM
python_modules/libraries/dagster-datadog/dagster_datadog/loggers.py
16–18 ↗(On Diff #10647)

oh that's great, didn't realize this was available

Also for context, WIP Datadog dashboard:

max added a comment.Mar 20 2020, 8:59 PM

I am not entirely clear on the advantage this has over a custom logger?

alangenfeld requested changes to this revision.Mar 23 2020, 6:35 PM

Q mgmt

This revision now requires changes to proceed.Mar 23 2020, 6:35 PM
sashank abandoned this revision.Mar 24 2020, 6:26 PM

In favor of custom logger