Skip to content

Commit cd63fc1

Browse files
jonaslbcmpadden
andauthored
fix(instigation logger): stringify non-json objects (#32867)
## Summary & Motivation Fixes #32845. Although, here is probably some underlying issue that is _not_ fixed by this PR. But I think that can go in another issue. ## How I Tested These Changes Implemented a test with a "weird" log record to show that logging does not error in this case any more. ## Changelog > InstigationLogger handles LogRecord attributes that are not json serializable by stringifying them instead of logging an error --------- Co-authored-by: Colton Padden <colton@dagsterlabs.com>
1 parent 4ffdd42 commit cd63fc1

2 files changed

Lines changed: 49 additions & 6 deletions

File tree

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@
77
from contextlib import ExitStack
88
from typing import IO, TYPE_CHECKING, Any, Optional
99

10-
from dagster_shared import seven
11-
1210
from dagster._core.log_manager import LOG_RECORD_METADATA_ATTR
1311
from dagster._core.storage.compute_log_manager import ComputeIOType, ComputeLogManager
1412
from dagster._core.utils import coerce_valid_log_level
@@ -73,10 +71,12 @@ def emit(self, record: logging.LogRecord):
7371
record_dict["exc_info"] = "".join(traceback.format_exception(*exc_info))
7472

7573
try:
76-
self._write_stream.write(seven.json.dumps(record_dict) + "\n")
74+
self._write_stream.write(json.dumps(record_dict, default=str, sort_keys=True) + "\n")
7775
except Exception:
7876
sys.stderr.write(
79-
f"Exception writing to logger event stream: {serializable_error_info_from_exc_info(sys.exc_info())}\n"
77+
f"Exception writing to logger event stream: {serializable_error_info_from_exc_info(sys.exc_info())}."
78+
f" The originating log call was made at: {record.pathname}:{record.funcName} (line {record.lineno})."
79+
"\n"
8080
)
8181

8282

@@ -179,7 +179,7 @@ def get_instigation_log_records(
179179
continue
180180

181181
try:
182-
records.append(seven.json.loads(line))
182+
records.append(json.loads(line, strict=False))
183183
except json.JSONDecodeError:
184184
continue
185185
return records

python_modules/dagster/dagster_tests/storage_tests/test_instigation_logger.py

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22
from unittest import mock
33

44
import dagster as dg
5-
from dagster._core.definitions.instigation_logger import InstigationLogger
5+
from dagster._core.definitions.instigation_logger import (
6+
InstigationLogger,
7+
get_instigation_log_records,
8+
)
69
from dagster._core.storage.noop_compute_log_manager import NoOpComputeLogManager
710

811

@@ -74,3 +77,43 @@ def test_instigation_logger_log_failure(capsys):
7477
captured.err.count("Exception closing logger write stream: Exception: OOPS ON EXIT")
7578
== 1
7679
)
80+
81+
82+
def test_instigation_logger_logs_weird_record_ok(capsys):
83+
class NonJsonSerializable:
84+
"""A class that json.dumps doesn't know what to do with (by default)."""
85+
86+
def __str__(self):
87+
return "non-jsonable-object"
88+
89+
log_key = ["test-repo", "test-instigator", "123"]
90+
91+
with dg.instance_for_test() as instance:
92+
with InstigationLogger(
93+
log_key=log_key,
94+
instance=instance,
95+
repository_name="test-repo",
96+
instigator_name="test-instigator",
97+
) as logger:
98+
logger.info("log without extras")
99+
logger.info("log with extras", extra={"non_json": NonJsonSerializable()})
100+
101+
assert logger.has_captured_logs()
102+
103+
captured = capsys.readouterr()
104+
105+
assert "Exception initializing logger write stream" not in captured.err
106+
assert "Exception writing to logger event stream" not in captured.err
107+
assert "Exception closing logger write stream" not in captured.err
108+
assert "NonJsonSerializable is not JSON serializable" not in captured.err
109+
110+
records = get_instigation_log_records(instance, log_key)
111+
112+
assert len(records) >= 2
113+
assert "test-repo - test-instigator - log with extras" in {
114+
record.get("msg") for record in records if record.get("msg")
115+
}
116+
matching_records = [record for record in records if record.get("non_json")]
117+
assert matching_records and matching_records[0]["non_json"] == "non-jsonable-object"
118+
119+
instance.compute_log_manager.delete_logs(log_key=log_key)

0 commit comments

Comments
 (0)