feat(ingest): add `--log-file` option and show CLI logs in UI report #7118

hsheth2 · 2023-01-24T07:31:04Z

Add a --log-file attribute that writes debug logs and stdout (e.g. the final ingestion report) to a file. This can even be used to dump debug logs to a file while printing info logs to the terminal
Includes the cli logs in the UI ingestion report. This currently matches the user's logging level for the cli run, but we can change that if we want to
Colorize logs based on severity in the CLI (unless stderr is redirected to a file)
Refactor logging setup

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

metadata-ingestion/src/datahub/utilities/logging_manager.py

treff7es

Lgtm

jjoyce0510 · 2023-01-26T02:09:36Z

metadata-ingestion/setup.py

@@ -122,6 +122,8 @@ def get_long_description():
    "greenlet",
 }

+sqllineage_lib = "sqllineage==1.3.6"


nice refactor!

jjoyce0510 · 2023-01-26T02:10:09Z

metadata-ingestion/src/datahub/cli/ingest_cli.py

@@ -393,5 +393,5 @@ def rollback(
                    writer.writerow([row.get("urn")])

        except IOError as e:
-            print(e)
+            logger.exception(f"Unable to save rollback failure report: {e}")


jjoyce0510 · 2023-01-26T02:10:20Z

metadata-ingestion/src/datahub/cli/migrate.py

@@ -262,7 +262,7 @@ def dataplatform2instance_func(
            delete_cli._delete_one_urn(src_entity_urn, soft=not hard, run_id=run_id)
        migration_report.on_entity_migrated(src_entity_urn, "status")  # type: ignore

-    print(f"{migration_report}")
+    click.echo(f"{migration_report}")


omg thank you

jjoyce0510 · 2023-01-26T02:15:57Z

metadata-ingestion/src/datahub/ingestion/reporting/datahub_ingestion_run_summary_provider.py

        execution_result_aspect = ExecutionRequestResultClass(
            status=status,
            startTimeMs=self.start_time_ms,
            durationMs=self.get_cur_time_in_ms() - self.start_time_ms,
-            report=json.dumps(report, indent=2),
+            report=summary,


Nice! So in this case, we are now sending the structured report to the correct field. From here, we can continue to add to that report and get it into our result aspect

(Sets ourselves up for much richer reports)

yup exactly - this way the structured report shows up in the correct field for both managed ingestion and cli ingestion

note that the structured report field is basically a string right now - we may need to rethink that in the future

jjoyce0510 · 2023-01-26T02:18:36Z

metadata-ingestion/src/datahub/utilities/logging_manager.py

+                return record.levelno >= logging.INFO
+        else:
+            if self.debug:
+                return record.levelno >= logging.WARNING


nice. good idea to filter out non-datahub noise

jjoyce0510 · 2023-01-26T02:23:02Z

metadata-ingestion/src/datahub/ingestion/reporting/datahub_ingestion_run_summary_provider.py

+        structured_report_str = json.dumps(report, indent=2)
+        summary = f"~~~~ Ingestion Report ~~~~\n{structured_report_str}\n\n"
+        summary += "~~~~ Ingestion Logs ~~~~\n"
+        summary += get_log_buffer().format_lines()


ah so here we leverage the in memory buffer. nice

jjoyce0510 · 2023-01-26T02:23:37Z

metadata-ingestion/src/datahub/utilities/logging_manager.py

+        # Configure the loggers.
+        root_logger = logging.getLogger()
+        _remove_all_handlers(root_logger)
+        root_logger.setLevel(logging.INFO)


why do we need this "normalization" step?

the default is warning globally, and can also be messed with by other libraries

jjoyce0510 · 2023-01-26T02:24:11Z

metadata-ingestion/src/datahub/utilities/logging_manager.py

+            # or otherwise mess with the logging configuration.
+            lib_logger = logging.getLogger(lib)
+            _remove_all_handlers(lib_logger)
+            lib_logger.setLevel(logging.DEBUG)


why this step?

jjoyce0510 · 2023-01-26T02:25:04Z

metadata-ingestion/src/datahub/utilities/tee_io.py

+from typing import Any, TextIO
+
+
+class TeeIO:


jjoyce0510 · 2023-01-26T02:27:26Z

metadata-ingestion/src/datahub/utilities/logging_manager.py

+        file_handler: logging.Handler
+        if log_file:
+            file = stack.enter_context(open(log_file, "w"))
+            tee = TeeIO(sys.stdout, file)


I'm curious - why is this Tee and context piece even required? I'd imagine we should be able to create a global handler that directs to a file without requiring this right?

we're redirecting stdout here (e.g. click.echo and print statements)

we want those messages to show up both in the terminal and in the log_file, which is why we need the tee

jjoyce0510 · 2023-01-26T02:28:07Z

metadata-ingestion/tests/unit/test_cli_logging.py

+    logger.warning("This is a warning message")
+    logger.error("this is an error with no stack trace")
+    try:
+        1 / 0


i like this

jjoyce0510 · 2023-01-26T02:28:53Z

metadata-ingestion/tests/unit/test_cli_logging.py

+        r"""\
+this is a print statement
+this is a click.echo statement
+[.+] DEBUG    .datahub.my_cli_module:\d+. - Example debug line


Ha! This is pretty cool

jjoyce0510 · 2023-01-26T02:29:10Z

metadata-ingestion/tests/unit/test_cli_logging.py

+[.+] INFO     .datahub.my_cli_module:\d+. - This is an info message
+[.+] WARNING  .datahub.my_cli_module:\d+. - This is a warning message
+[.+] ERROR    .datahub.my_cli_module:\d+. - this is an error with no stack trace
+[.+] ERROR    .datahub.my_cli_module:\d+. - failed to divide by zero


Why does everything start with [.+]?

to handle the timestamp part of the log

jjoyce0510 · 2023-01-26T02:29:51Z

metadata-ingestion/tests/unit/test_cli_logging.py

+    # The in-memory log buffer should contain the log messages.
+    # The first two lines are stdout, so we skip them.
+    expected_log_output = "\n".join(result.output.splitlines()[2:])
+    assert get_log_buffer().format_lines() == expected_log_output


Do we need to cleanup the file here?

nope its a pytest tmp_file so it gets cleaned up automatically

jjoyce0510

LGTM

…atahub-project#7118) Co-authored-by: Tamas Nemeth <treff7es@gmail.com>

start log-file handler

f5595a0

github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Jan 24, 2023

hsheth2 and others added 6 commits January 23, 2023 23:47

update docs

308428f

Merge branch 'master' into log-file

3954c51

Merge branch 'master' into log-file

b1c3d60

test in-memory buffer

4e36436

Merge branch 'master' into log-file

efb4d24

fix test

3e22c5e

treff7es reviewed Jan 25, 2023

View reviewed changes

metadata-ingestion/src/datahub/utilities/logging_manager.py Show resolved Hide resolved

treff7es approved these changes Jan 25, 2023

View reviewed changes

support NO_COLOR

62a890d

jjoyce0510 reviewed Jan 26, 2023

View reviewed changes

metadata-ingestion/src/datahub/utilities/tee_io.py

from typing import Any, TextIO

class TeeIO:

Copy link

Collaborator

jjoyce0510 Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fancy!

jjoyce0510 reviewed Jan 26, 2023

View reviewed changes

jjoyce0510 approved these changes Jan 26, 2023

View reviewed changes

hsheth2 merged commit 927d45d into datahub-project:master Jan 26, 2023

hsheth2 deleted the log-file branch January 26, 2023 17:25

ericyomi pushed a commit to ericyomi/datahub that referenced this pull request Feb 8, 2023

feat(ingest): add --log-file option and show CLI logs in UI report (d…

43a04a9

…atahub-project#7118) Co-authored-by: Tamas Nemeth <treff7es@gmail.com>

oleg-ruban pushed a commit to RChygir/datahub that referenced this pull request Feb 28, 2023

feat(ingest): add --log-file option and show CLI logs in UI report (d…

3e0ffd3

…atahub-project#7118) Co-authored-by: Tamas Nemeth <treff7es@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ingest): add `--log-file` option and show CLI logs in UI report #7118

feat(ingest): add `--log-file` option and show CLI logs in UI report #7118

hsheth2 commented Jan 24, 2023

treff7es left a comment

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

hsheth2 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

hsheth2 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

hsheth2 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 Jan 26, 2023

hsheth2 Jan 26, 2023

jjoyce0510 Jan 26, 2023

hsheth2 Jan 26, 2023

jjoyce0510 Jan 26, 2023

jjoyce0510 left a comment

feat(ingest): add --log-file option and show CLI logs in UI report #7118

feat(ingest): add --log-file option and show CLI logs in UI report #7118

Conversation

hsheth2 commented Jan 24, 2023

Checklist

treff7es left a 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

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

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

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

jjoyce0510 left a comment

Choose a reason for hiding this comment

feat(ingest): add `--log-file` option and show CLI logs in UI report #7118

feat(ingest): add `--log-file` option and show CLI logs in UI report #7118