[Core] [Logging] | Integration logs not being ingested #1120

ivankalinovski · 2024-11-05T16:12:04Z

Description

What:

Fix logs not being ingested on integrations fails or exceptions.

Why:

The logger flush method is spawning a new thread that is not awaited when the program exists.
The logger accepts 'exc_info' that cannot be serialized.
Upon shutdown of the program the logger writes a message that needs to be awaited in order to be sent to Port API.

How:

Added method - 'wait_for_lingering_threads' to 'HTTPMemoryHandler' that will wait for running threads.
'wait_for_lingering_threads' is invoked via the 'SignalHandler' upon exit.
Modified method - '_serialize_record' to look for 'exc_info' in each log and serialize it using 'traceback'.
Added call - 'logger.complete' so log that is sent on exit of program will be sent to Port API.

Type of change

Please leave one option from the following and delete the rest:

Bug fix (non-breaking change which fixes an issue)

All tests should be run against the port production environment(using a testing org).

Core testing checklist

Integration able to create all default resources from scratch
Resync finishes successfully
Resync able to create entities
Resync able to update entities
Resync able to detect and delete entities
Scheduled resync able to abort existing resync and start a new one
Tested with at least 2 integrations from scratch
Tested with Kafka and Polling event listeners
Tested deletion of entities that don't pass the selector

Screenshots

Include screenshots from your environment showing how the resources of the integration will look.

API Documentation

Provide links to the API documentation used for this integration.

…xception on exit. 3.Sirealize exception.

Tankilevitch

Great to have you contributing to ocean 🌊 and diving straight to a painful bug 🤩
Left a few comments, also make sure you update the version in the pyproject.toml as well :)

port_ocean/clients/port/mixins/integrations.py

Tankilevitch · 2024-11-05T16:22:56Z

port_ocean/clients/port/mixins/integrations.py

    async def ingest_integration_logs(self, logs: list[dict[str, Any]]) -> None:
        logger.debug("Ingesting logs")
+        sirealized_logs=self.sirealize_logs(logs)


Suggested change

sirealized_logs=self.sirealize_logs(logs)

serialized_logs=self.serialize_logs(logs)

Tankilevitch · 2024-11-05T16:25:23Z

port_ocean/clients/port/mixins/integrations.py

@@ -99,15 +100,24 @@ async def patch_integration(
        handle_status_code(response)
        return response.json()["integration"]

+    def sirealize_logs(self,logs: list[dict[str, Any]]):


This seems like a pretty isolated method, lets add a test for it

Tankilevitch · 2024-11-05T16:27:04Z

port_ocean/log/handlers.py

+            clear_thread_pool()
+            thread=threading.Thread(target=_wrap_event_loop, args=(self.ocean, logs))
+            thread.start()
+            self._thread_pool.append(thread)


are we supporting multiple threads flushing logs concurrently?
lets test long running, scheduled resync to verify we didn't break anything.

# Description What - bump fastapi version to 0.115.3 to upgrade starlette Why - https://github.com/port-labs/ocean/security/dependabot/433 How - bump fastapi version ## Type of change Please leave one option from the following and delete the rest: - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] New Integration (non-breaking change which adds a new integration) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [X] Non-breaking change (fix of existing functionality that will not change current behavior) - [ ] Documentation (added/updated documentation) <h4> All tests should be run against the port production environment(using a testing org). </h4> ### Core testing checklist - [x] Integration able to create all default resources from scratch - [x] Resync finishes successfully - [x] Resync able to create entities - [x] Resync able to update entities - [x] Resync able to detect and delete entities - [x] Scheduled resync able to abort existing resync and start a new one - [x] Tested with at least 2 integrations from scratch - [x] Tested with Kafka and Polling event listeners - [x] Tested deletion of entities that don't pass the selector ### Integration testing checklist - [ ] Integration able to create all default resources from scratch - [ ] Resync able to create entities - [ ] Resync able to update entities - [ ] Resync able to detect and delete entities - [ ] Resync finishes successfully - [ ] If new resource kind is added or updated in the integration, add example raw data, mapping and expected result to the `examples` folder in the integration directory. - [ ] If resource kind is updated, run the integration with the example data and check if the expected result is achieved - [ ] If new resource kind is added or updated, validate that live-events for that resource are working as expected - [ ] Docs PR link [here](#) ### Preflight checklist - [ ] Handled rate limiting - [ ] Handled pagination - [ ] Implemented the code in async - [ ] Support Multi account ## Screenshots Include screenshots from your environment showing how the resources of the integration will look. ## API Documentation Provide links to the API documentation used for this integration. --------- Co-authored-by: Shalev Avhar <[email protected]> Co-authored-by: Tom Tankilevitch <[email protected]>

This PR was automatically created by a GitHub Action. ## What does this PR do? Apply Ocean version 0.12.8 to all integrations ## How should this be manually tested? ./scripts/bump-all.sh ^0.12.8 --------- Co-authored-by: GitHub Action <[email protected]> Co-authored-by: Shalev Avhar <[email protected]>

erikzaadi

Be sure to rebase to fix your conflict
The commit messages themselves are a bit long, put the main change list you added there to the commit body
Besides that 3 small NITs
Approving in the meanwhile to save you a hop, be sure to validate the testing Q @Tankilevitch asked

erikzaadi · 2024-11-07T08:28:08Z

port_ocean/tests/log/test_handlers.py

+
+
+def assert_extra(extra: dict):
+    assert "exc_info" in extra


Suggested change

assert "exc_info" in extra

exc_info = extra.get("exc_info", None)

assert type(exec_info) is str

return exec_info

erikzaadi · 2024-11-07T08:28:33Z

port_ocean/tests/log/test_handlers.py

+    return exc_info
+
+
+def log_record(cb: Callable[[None], None]) -> "loguru.Record":


Why a string based type?

erikzaadi · 2024-11-07T08:29:15Z

integrations/gitlab/CHANGELOG.md

+
+- Await logger writing exception on exit (Integration logs not being ingested)
+- Await logger thread on exit (Integration logs not being ingested)
+- Sirealize exception (Integration logs not being ingested)


git grep -i sirealize

…xception on exit. 3.Sirealize exception.

1.Extend method "_serialize_record" to stringify exceptions in "exc_info". 2.Await logger on thread exit. 3.Await logger writing exception on exit.

Ivan Kalinovski added 2 commits November 5, 2024 16:54

PORT-10719 1.Await to logger thread on exit. 2.Await logger writing e…

836439d

…xception on exit. 3.Sirealize exception.

PORT-10719 add changelog

6f19c92

ivankalinovski requested a review from a team as a code owner November 5, 2024 16:12

github-actions bot added the size/S label Nov 5, 2024

PORT-10719 1.fix lint 2.change function name to serialize_logs

6c3a232

Tankilevitch reviewed Nov 5, 2024

View reviewed changes

shalev007 and others added 3 commits November 6, 2024 11:02

PORT-10719 1.Move serialize_logs into _serialize_record. 2.Add tests.

0dad786

github-actions bot added size/M and removed size/S labels Nov 7, 2024

erikzaadi approved these changes Nov 7, 2024

View reviewed changes

Ivan Kalinovski added 6 commits November 7, 2024 10:42

PORT-10719 1.Await to logger thread on exit. 2.Await logger writing e…

6419b8e

…xception on exit. 3.Sirealize exception.

PORT-10719 add changelog

9683385

PORT-10719 1.fix lint 2.change function name to serialize_logs

685261f

PORT-10719 1.Move serialize_logs into _serialize_record. 2.Add tests.

6553c58

PORT-10719 logs not ingested

52bd7f2

1.Extend method "_serialize_record" to stringify exceptions in "exc_info". 2.Await logger on thread exit. 3.Await logger writing exception on exit.

PORT-10719 logs not ingested

b450e5b

1.Extend method "_serialize_record" to stringify exceptions in "exc_info". 2.Await logger on thread exit. 3.Await logger writing exception on exit.

github-actions bot added size/XXL and removed size/M labels Nov 7, 2024

Integration logs not being ingested

db55df7

1.Extend method "_serialize_record" to stringify exceptions in "exc_info". 2.Await logger on thread exit. 3.Await logger writing exception on exit.

ivankalinovski merged commit 0faaf0d into main Nov 7, 2024
74 checks passed

ivankalinovski deleted the PORT-10719-bug-integration-logs-not-being-ingested branch November 7, 2024 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] [Logging] | Integration logs not being ingested #1120

[Core] [Logging] | Integration logs not being ingested #1120

ivankalinovski commented Nov 5, 2024 •

edited

Loading

Tankilevitch left a comment

Tankilevitch Nov 5, 2024

Tankilevitch Nov 5, 2024

Tankilevitch Nov 5, 2024

erikzaadi left a comment

erikzaadi Nov 7, 2024

erikzaadi Nov 7, 2024

erikzaadi Nov 7, 2024

	sirealized_logs=self.sirealize_logs(logs)
	serialized_logs=self.serialize_logs(logs)

-    assert "exc_info" in extra
+    exc_info = extra.get("exc_info", None)
+    assert type(exec_info) is str
+    return exec_info

		return exc_info


		def log_record(cb: Callable[[None], None]) -> "loguru.Record":

[Core] [Logging] | Integration logs not being ingested #1120

[Core] [Logging] | Integration logs not being ingested #1120

Conversation

ivankalinovski commented Nov 5, 2024 • edited Loading

Description

Type of change

All tests should be run against the port production environment(using a testing org).

Core testing checklist

Screenshots

API Documentation

Tankilevitch left a comment

Choose a reason for hiding this comment

Tankilevitch Nov 5, 2024

Choose a reason for hiding this comment

Tankilevitch Nov 5, 2024

Choose a reason for hiding this comment

Tankilevitch Nov 5, 2024

Choose a reason for hiding this comment

erikzaadi left a comment

Choose a reason for hiding this comment

erikzaadi Nov 7, 2024

Choose a reason for hiding this comment

erikzaadi Nov 7, 2024

Choose a reason for hiding this comment

erikzaadi Nov 7, 2024

Choose a reason for hiding this comment

ivankalinovski commented Nov 5, 2024 •

edited

Loading