Page MenuHomeElementl

add log capture event
ClosedPublic

Authored by prha on May 17 2021, 5:44 PM.

Details

Summary

Right now, the Run view and the compute log manager have an implicit agreement about the structure of compute logs. The run view assumes that every run's compute logs are captured by step, and adds links to the raw compute logs in each step start event.

In order to have different granularity of compute log capture, this diff adds a distinct log capture event, along with a log key to query by. This should allow us to emit a log capture event per step to maintain the existing per-step capture behavior, but also add a per-process capture event, along with a distinct log key.

old event:

new event:

Test Plan

bk

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 17 2021, 6:06 PM
Harbormaster failed remote builds in B30650: Diff 37690!
Harbormaster returned this revision to the author for changes because remote builds failed.May 17 2021, 7:26 PM
Harbormaster failed remote builds in B30660: Diff 37702!
Harbormaster returned this revision to the author for changes because remote builds failed.May 17 2021, 7:48 PM
Harbormaster failed remote builds in B30664: Diff 37706!
prha requested review of this revision.May 17 2021, 11:58 PM

I thiiiiink this is legit

This should allow us to emit a log capture event per step to maintain the existing per-step capture behavior, but also add a per-process capture event, along with a distinct log key.

Do you have a diff / prototype for this yet? Just wondering if we should prove out these serdes objets before they start getting put in the DB

js_modules/dagit/packages/core/src/runs/LogsRowStructuredContent.tsx
209

maybe we should move away from "Step" since we don't really indicate thats a thing consistently

  • view logs
  • View stdout/stderr
  • view captured log output
  • view captured text output
  • view raw logs
python_modules/dagster/dagster/core/events/__init__.py
1133

mypy

1136

maybe "for solid" - same reasoning as above

python_modules/dagster/dagster/core/execution/plan/execute_plan.py
65–67

maybe kwargs here since step key / log key is subtle

Seems fine, though it's hard for me to tell what exactly the changes are in Dagit. Can you add a screenshot or two?

js_modules/dagit/packages/core/src/runs/LogsToolbar.tsx
106–117

Could probably consolidate this into a single loop.

109

This is identical to the some call above.

This revision is now accepted and ready to land.May 25 2021, 5:50 PM
python_modules/dagster-graphql/dagster_graphql/schema/logs/events.py
398–399

certain these will always be not null?

python_modules/dagster-graphql/dagster_graphql/schema/logs/events.py
398–399

I think pid will always be not null. Perhaps stepKeys should be nullable, instead of empty-able.

This revision was automatically updated to reflect the committed changes.