Page MenuHomePhabricator
Differential D5402

[RFC] GraphQL endpoint for daemon statuses
ClosedPublic
Actions

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

Details

Reviewers
prha
dish
dgibson
Commits
R1:fc3efd7bd9d9: [RFC] GraphQL endpoint for daemon statuses
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 created this revision.Dec 3 2020, 2:24 AM
johann edited the summary of this revision. (Show Details)Dec 3 2020, 2:31 AM
johann edited the test plan for this revision. (Show Details)
johann added a comment.Dec 3 2020, 2:35 AM

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 edited the summary of this revision. (Show Details)Dec 3 2020, 2:36 AM
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!
johann updated this revision to Diff 26889.Dec 3 2020, 2:52 AM

up

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!
johann updated this revision to Diff 26890.Dec 3 2020, 3:25 AM

up

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)Dec 3 2020, 3:54 AM
johann updated this revision to Diff 26891.Dec 3 2020, 3:54 AM
johann edited the summary of this revision. (Show Details)

up

Harbormaster completed remote builds in B22142: Diff 26891.Dec 3 2020, 4:12 AM
johann requested review of this revision.Dec 3 2020, 4:12 AM
prha added inline comments.Dec 3 2020, 4:42 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
johann updated this revision to Diff 26972.Dec 4 2020, 3:22 PM

update schema and add test

johann updated this revision to Diff 26973.Dec 4 2020, 3:36 PM

clean up controller

dgibson accepted this revision.Dec 4 2020, 3:37 PM

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
johann updated this revision to Diff 26975.Dec 4 2020, 3:38 PM

add timezone

Harbormaster failed remote builds in B22208: Diff 26972!Dec 4 2020, 3:40 PM
Harbormaster failed remote builds in B22209: Diff 26973!Dec 4 2020, 3:52 PM
Harbormaster failed remote builds in B22211: Diff 26975!Dec 4 2020, 3:54 PM
johann updated this revision to Diff 26996.Dec 4 2020, 6:16 PM

DaemonType enum

Harbormaster failed remote builds in B22227: Diff 26996!Dec 4 2020, 6:32 PM
johann updated this revision to Diff 26999.Dec 4 2020, 6:48 PM

rebase

Harbormaster completed remote builds in B22228: Diff 26999.Dec 4 2020, 7:06 PM
johann updated this revision to Diff 27010.Dec 4 2020, 8:03 PM

use dauphin from_enum

johann updated this revision to Diff 27011.Dec 4 2020, 8:06 PM

typo

Harbormaster failed remote builds in B22236: Diff 27010!Dec 4 2020, 8:21 PM
Harbormaster completed remote builds in B22237: Diff 27011.
Closed by commit R1:fc3efd7bd9d9: [RFC] GraphQL endpoint for daemon statuses (authored by johann). · Explain WhyDec 4 2020, 9:01 PM
This revision was automatically updated to reflect the committed changes.
johann added a commit: R1:fc3efd7bd9d9: [RFC] GraphQL endpoint for daemon statuses.

Revision Contents
Changeset List

PathSize
js_modules/
dagit/
src/
19 lines
python_modules/
dagster-graphql/
dagster_graphql/
schema/
70 lines
dagster_graphql_tests/
graphql/
109 lines
dagster/
dagster/
core/
instance/
17 lines
storage/
runs/
13 lines
daemon/
cli/
4 lines
125 lines
17 lines
run_coordinator/
5 lines
25 lines
dagster_tests/
core_tests/
storage_tests/
utils/
16 lines
daemon_tests/
integration_tests/
8 lines
28 lines
libraries/
dagster-postgres/
dagster_postgres/
run_storage/
2 lines

Diff 27021

View Options

js_modules/dagit/src/schema.graphql

Loading...
View Options

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

Loading...
View Options

python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_daemon_health.py

Loading...
View Options

python_modules/dagster/dagster/core/instance/__init__.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/runs/sql_run_storage.py

Loading...
View Options

python_modules/dagster/dagster/daemon/cli/__init__.py

Loading...
View Options

python_modules/dagster/dagster/daemon/controller.py

Loading...
View Options

python_modules/dagster/dagster/daemon/daemon.py

Loading...
View Options

python_modules/dagster/dagster/daemon/run_coordinator/queued_run_coordinator_daemon.py

Loading...
View Options

python_modules/dagster/dagster/daemon/types.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/storage_tests/utils/run_storage.py

Loading...
View Options

python_modules/dagster/dagster_tests/daemon_tests/integration_tests/test_daemon.py

Loading...
View Options

python_modules/dagster/dagster_tests/daemon_tests/test_dagster_daemon.py

Loading...
View Options

python_modules/libraries/dagster-postgres/dagster_postgres/run_storage/run_storage.py

Loading...