use compute log manager to store backfill daemon logs for UI display #21786

jamiedemaria · 2024-05-10T18:22:44Z

Summary & Motivation

proof of concept PR to look at how we can get backfill daemon logs surfaced to the UI. I left comments throughout the PR with some open design questions. If it is more convenient to do a meeting to chat through all of this, I'm happy to put that together

How I Tested These Changes

jamiedemaria · 2024-05-10T18:22:58Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

Access backfill ticks in GQL #21787
run requests and submitted runs on tick #21794
use compute log manager to store backfill daemon logs for UI display #21786 👈
add backfills to the ticks table #21750
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @jamiedemaria and the rest of your teammates on Graphite

jamiedemaria · 2024-05-14T15:12:15Z

python_modules/dagster/dagster/_daemon/backfill.py

-                )
+        evaluation_time = pendulum.now("UTC")
+
+        # TODO - probably need to scope by code location. for now just put everything together for testing


before merge - align on a log key scheme

jamiedemaria · 2024-05-14T15:15:07Z

python_modules/dagster/dagster/_daemon/backfill.py

+
+        # TODO - probably need to scope by code location. for now just put everything together for testing
+        log_key = ["backfill", backfill_id, evaluation_time.strftime("%Y%m%d_%H%M%S")]
+        with InstigationLogger(


From my understanding, the InstigationLogger has only been used for things like sensor logs where the logs are user defined. Since we want to have our backfill daemon logs appear in the UI, that could change some of the assumptions made by the InstigationLogger

the InstigationLogger always uses the dagster logger, whereas in this case we want the backfill daemon logger

dagster/python_modules/dagster/dagster/_core/definitions/instigation_logger.py

Line 92 in 97c80ef

super().__init__(name="dagster", level=coerce_valid_log_level(level))

jamiedemaria · 2024-05-14T15:17:07Z

python_modules/dagster/dagster/_daemon/backfill.py

+
+        # TODO - probably need to scope by code location. for now just put everything together for testing
+        log_key = ["backfill", backfill_id, evaluation_time.strftime("%Y%m%d_%H%M%S")]
+        with InstigationLogger(


sensor logs are stored by the user, for OSS we can use the same storage, but for plus we will likely need to store the logs ourselves. my homework for today is to understand how we would handle this in plus, but any ideas/opinions on how to manage this from those with more platform experience would be super helpful

The use of the compute log manager for sensor logs is kind of a hack / implementation detail.

Compute logs are user-configured, and for a lot of our customers are not viable because they want a hard separation between Dagster and their computation layer.

We're almost better off with a new instance API call like storeLogMessage or something like that. In Cloud, that thing can be backed by something like some S3 storage (that is different than the user-configured compute log manager) so that these messages never touch Postgres.

In OSS, we can use the compute log manager or another configured instance class to put these logs someplace durable.

jamiedemaria · 2024-05-14T15:21:20Z

python_modules/dagster/dagster/_daemon/backfill.py

+
+        # TODO - probably need to scope by code location. for now just put everything together for testing
+        log_key = ["backfill", backfill_id, evaluation_time.strftime("%Y%m%d_%H%M%S")]
+        with InstigationLogger(


To get these logs available in the UI, I had to do add some special logic for how to get the message from the log record https://github.com/dagster-io/dagster/pull/21787/files#diff-a2c3875505d59d1c50f93bbb312c943044d849e91c319d5349b38f57947ff16eR58-R62 because the _annotate_record of the InstigationLogger doesn't seem to get applied

dagster/python_modules/dagster/dagster/_core/definitions/instigation_logger.py

Line 120 in 97c80ef

def _annotate_record(self, record: logging.LogRecord) -> logging.LogRecord:

more homework for myself today is to figure out why this is happening so that we can ideally not have that special behavior in the GQL layer

hmm, yeah, our logger overrides is really clunky. It's been a while, but I think it's because we want to use the Logger interface for something that's really a LogHandler.

jamiedemaria · 2024-05-14T15:28:33Z

@alangenfeld @gibsondan @prha like #21750 i'm marking this ready for review to get it in review queues for the open questions. it's definitely needs a lot of work before it could actually be merged, and i appreciate you looking through my kind of hacky code.

one thing i'm curious to get opinions on is if the issues i left in the comments are reasons we should diverge from the InstigationLogger and do something more specific to this daemon logging system. i'm kind of learning about logging as i go with this so i'm not up to speed on our logging history so i'm not sure what this history and intentions of theInstigationLogger are.

prha

Happy to do a quick sync on this.

prha · 2024-05-15T15:13:20Z

python_modules/dagster/dagster/_daemon/backfill.py

+
+        # TODO - probably need to scope by code location. for now just put everything together for testing
+        log_key = ["backfill", backfill_id, evaluation_time.strftime("%Y%m%d_%H%M%S")]
+        with InstigationLogger(


The use of the compute log manager for sensor logs is kind of a hack / implementation detail.

Compute logs are user-configured, and for a lot of our customers are not viable because they want a hard separation between Dagster and their computation layer.

We're almost better off with a new instance API call like storeLogMessage or something like that. In Cloud, that thing can be backed by something like some S3 storage (that is different than the user-configured compute log manager) so that these messages never touch Postgres.

In OSS, we can use the compute log manager or another configured instance class to put these logs someplace durable.

prha · 2024-05-15T15:18:56Z

python_modules/dagster/dagster/_daemon/backfill.py

+
+        # TODO - probably need to scope by code location. for now just put everything together for testing
+        log_key = ["backfill", backfill_id, evaluation_time.strftime("%Y%m%d_%H%M%S")]
+        with InstigationLogger(


hmm, yeah, our logger overrides is really clunky. It's been a while, but I think it's because we want to use the Logger interface for something that's really a LogHandler.

alangenfeld

to your queue

jamiedemaria · 2024-05-28T13:57:23Z

stack is deprecated in favor of #22003. closing

jamiedemaria mentioned this pull request May 10, 2024

add backfills to the ticks table #21750

Closed

This was referenced May 10, 2024

Access backfill ticks in GQL #21787

Closed

run requests and submitted runs on tick #21794

Closed

jamiedemaria changed the title ~~wip - backfill logging through to ui~~ prototype - use compute log manager to store backfill daemon logs for UI display May 13, 2024

jamiedemaria commented May 14, 2024

View reviewed changes

jamiedemaria requested review from alangenfeld, prha and gibsondan May 14, 2024 15:21

jamiedemaria marked this pull request as ready for review May 14, 2024 15:28

jamiedemaria changed the title ~~prototype - use compute log manager to store backfill daemon logs for UI display~~ use compute log manager to store backfill daemon logs for UI display May 14, 2024

prha reviewed May 15, 2024

View reviewed changes

jamiedemaria added 4 commits May 17, 2024 11:52

trivial

6d8ac55

prototype storing logs

629f144

test

29668d1

back

e75ef3d

jamiedemaria force-pushed the jamie/backfill-ticks branch from 6c7061b to 514eae3 Compare May 17, 2024 15:57

jamiedemaria force-pushed the jamie/daemon-logger-with-log-key branch from 7ff30c5 to e75ef3d Compare May 17, 2024 15:57

alangenfeld requested changes May 24, 2024

View reviewed changes

jamiedemaria marked this pull request as draft May 28, 2024 13:56

jamiedemaria closed this May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use compute log manager to store backfill daemon logs for UI display #21786

use compute log manager to store backfill daemon logs for UI display #21786

jamiedemaria commented May 10, 2024 •

edited

jamiedemaria commented May 10, 2024 •

edited

jamiedemaria May 14, 2024

jamiedemaria May 14, 2024

jamiedemaria May 14, 2024

prha May 15, 2024

jamiedemaria May 14, 2024

prha May 15, 2024

jamiedemaria commented May 14, 2024

prha left a comment

prha May 15, 2024

prha May 15, 2024

alangenfeld left a comment

jamiedemaria commented May 28, 2024

use compute log manager to store backfill daemon logs for UI display #21786

use compute log manager to store backfill daemon logs for UI display #21786

Conversation

jamiedemaria commented May 10, 2024 • edited

Summary & Motivation

How I Tested These Changes

jamiedemaria commented May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiedemaria commented May 14, 2024

prha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alangenfeld left a comment

Choose a reason for hiding this comment

jamiedemaria commented May 28, 2024

jamiedemaria commented May 10, 2024 •

edited

jamiedemaria commented May 10, 2024 •

edited