Page MenuHomePhabricator

[RFC] GraphQL endpoint for daemon statuses
ClosedPublic

Authored by johann on Dec 3 2020, 2:24 AM.

Details

Summary

Fetch info about the running daemons.

This made a mess in controller.py, I'll consolidate the methods there before landing

query InstanceDetailSummaryQuery {
  version
  instance {
    daemonHealth {
      sensorDaemon {
        required
        healthy
        lastHeartbeatTime
      }
    }
  }
}

{
  "data": {
    "version": "0.9.20",
    "instance": {
      "daemonHealth": {
        "sensorDaemon": {
          "required": true,
          "healthy": true,
          "lastHeartbeatTime": 1606962000.61016
        }
      }
    }
  }
}
Test Plan

TODO!

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

johann edited the test plan for this revision. (Show Details)

Per a discussion with Dish, we decided to have individual methods for daemon health rather than a list, since we will want to single out particular daemons on respective pages.

Is there anything I should do to prevent the daemons in graphql and in the controller from going out of sync? Currently a new daemon wouldn't be surfaced in graphql at all, and if one of these daemons were renamed, then graphql would always report it as not required.

johann added reviewers: prha, dish, dgibson.
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 3 2020, 2:43 AM
Harbormaster failed remote builds in B22139: Diff 26887!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 3 2020, 3:11 AM
Harbormaster failed remote builds in B22140: Diff 26889!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 3 2020, 3:46 AM
Harbormaster failed remote builds in B22141: Diff 26890!
johann edited the summary of this revision. (Show Details)

up

johann requested review of this revision.Dec 3 2020, 4:12 AM
python_modules/dagster-graphql/dagster_graphql/schema/roots.py
989

for naming consistency, should this be schedulerDaemon?

989–991

if you put daemon type as a field on DaemonStatus, you could still have these individual daemon fields, but also an allDaemons field that returned a non_null_list of DaemonStatus.

That way if the number of daemons changed, you'd know.

dgibson requested changes to this revision.Dec 3 2020, 5:10 PM
dgibson added inline comments.
js_modules/dagit/src/schema.graphql
889–908

Giving dagit the ability to query for individual daemons of a specific type seems fine but I'm not sure we need a separate field for each one - could we use a DaemonType enum that we pass in as an argument?

e.g.

daemonHealth(DaemonType.SENSOR)?

(Looks like prha suggested this later)

python_modules/dagster-graphql/dagster_graphql/schema/roots.py
977–1042

oh here it is! yeah pass one of these in :)

python_modules/dagster/dagster/daemon/controller.py
160

we should use a timezone for this or it will default to the system timezone, which may be different than the daemon's timezone

This revision now requires changes to proceed.Dec 3 2020, 5:10 PM

update schema and add test

mostly small things / suggestions. Be sure to fix the timezone issue

js_modules/dagit/src/schema.graphql
900–902

SCHEDULER / SENSOR / QUEUED_RUN_COORDINATOR? The fact that they're Daemons is clear from context

python_modules/dagster-graphql/dagster_graphql/schema/roots.py
977

is this inline import needed? I don't think dagster core depends on dagster-graphql

977–985

Consider making this a map of DauphinDaemonTypes to python DaemonTypes (or alternately to the class name strings) and waiting until the various resolve_XXX methods to actually instantiate any Dauphin objects, then you can use graphene_info in the standard way

something like

def _get_dauphin_daemon_status(self, instance,graphene_info, daemon_type) {
   return graphene_info.schema.types_named("DaemonStatus")(get_daemon_status(instance, daemon_type))
}
1001–1003

consider keying on a separate python Enum on the DaemonStatus class rather than the class name

python_modules/dagster/dagster/daemon/controller.py
159–162

I think a python daemontype enum would make some of this cleaner and more type safe, there's lots of switching back and forth between class names and enums in the graphql code

160

this comment still applies - what timezone is the daemon controller using? If it's also using pendulum.now we should switch it to UTC, and then this should be pendulum.now("UTC")

This revision is now accepted and ready to land.Dec 4 2020, 3:37 PM
This revision was automatically updated to reflect the committed changes.