Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent bootlooping due to a failed plugin load. #73

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Prevent bootlooping due to a failed plugin load. #73

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions plugin_runner/plugin_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ def load_or_reload_plugin(path: pathlib.Path) -> None:
try:
manifest_json = json.loads(manifest_json)
except Exception as e:
log.warning(f'Unable to load plugin "{name}": {e}')
log.error(f'Unable to load plugin "{name}": {e}')
return

secrets_file = path / SECRETS_FILE_NAME
Expand All @@ -162,13 +162,13 @@ def load_or_reload_plugin(path: pathlib.Path) -> None:
try:
secrets_json = json.load(secrets_file.open())
except Exception as e:
log.warning(f'Unable to load secrets for plugin "{name}": {str(e)}')
log.error(f'Unable to load secrets for plugin "{name}": {str(e)}')

# TODO add existing schema validation from Michela here
try:
protocols = manifest_json["components"]["protocols"]
except Exception as e:
log.warning(f'Unable to load plugin "{name}": {str(e)}')
log.error(f'Unable to load plugin "{name}": {str(e)}')
return

for protocol in protocols:
Expand Down Expand Up @@ -204,8 +204,10 @@ def load_or_reload_plugin(path: pathlib.Path) -> None:
"protocol": protocol,
"secrets": secrets_json,
}
except ImportError as err:
except Exception as err:
log.error(f"Error importing module '{name_and_class}': {err}")
for error_line in traceback.format_exception(err):
Copy link
Member

Choose a reason for hiding this comment

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

hmm why not just pass the whole exception to log.error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was a design goal to accomplish this in the most silly way possible. (I will fix)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well wait, we do that above in the string interpolation. It doesn't show the whole stack. I would like the log output to show the line number of the offending class.

Copy link
Member

Choose a reason for hiding this comment

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

you would still call format_exception, but just pass the output to log.error directly rather than every line... let me test real quick if it actually does what I think

Copy link
Member

Choose a reason for hiding this comment

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

ok I stand corrected, how about this:

log.error('\n'.join(traceback.format_exception(err))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might just be a preference thing, but yours messes with the indent due to the single log timestamp vs one per line.

yours:

ERROR 2024-07-16 10:54:10,033 Error importing module 'search:search.protocols.condition:Protocol': EnumTypeWrapper.Name() takes 2 positional arguments but 3 were given
ERROR 2024-07-16 10:54:10,034 Traceback (most recent call last):

  File "/Users/andrew/src/canvas-plugins/plugin_runner/plugin_runner.py", line 198, in load_or_reload_plugin
    result = sandbox_from_module_name(protocol_module)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  File "/Users/andrew/src/canvas-plugins/plugin_runner/plugin_runner.py", line 139, in sandbox_from_module_name
    return sandbox.execute()
           ^^^^^^^^^^^^^^^^^

  File "/Users/andrew/src/canvas-plugins/plugin_runner/sandbox.py", line 267, in execute
    exec(self.compile_result.code, self.scope)

  File "<string>", line 1, in <module>

  File "<string>", line 14, in Protocol

TypeError: EnumTypeWrapper.Name() takes 2 positional arguments but 3 were given

mine:

ERROR 2024-07-16 10:25:40,695 Error importing module 'search:search.protocols.condition:Protocol': EnumTypeWrapper.Name() takes 2 positional arguments but 3 were given
ERROR 2024-07-16 10:25:40,696 Traceback (most recent call last):

ERROR 2024-07-16 10:25:40,696   File "/Users/andrew/src/canvas-plugins/plugin_runner/plugin_runner.py", line 198, in load_or_reload_plugin
    result = sandbox_from_module_name(protocol_module)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

ERROR 2024-07-16 10:25:40,696   File "/Users/andrew/src/canvas-plugins/plugin_runner/plugin_runner.py", line 139, in sandbox_from_module_name
    return sandbox.execute()
           ^^^^^^^^^^^^^^^^^

ERROR 2024-07-16 10:25:40,696   File "/Users/andrew/src/canvas-plugins/plugin_runner/sandbox.py", line 267, in execute
    exec(self.compile_result.code, self.scope)

ERROR 2024-07-16 10:25:40,696   File "<string>", line 1, in <module>

ERROR 2024-07-16 10:25:40,696   File "<string>", line 14, in Protocol

ERROR 2024-07-16 10:25:40,696 TypeError: EnumTypeWrapper.Name() takes 2 positional arguments but 3 were given

Which do you prefer? I don't feel strongly.

Copy link
Member

Choose a reason for hiding this comment

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

i have a very slight preference for mine because it's easier to read i think... on the flip side it would be impossible to search for in kibana (whereas with your method you could search for all lines that start with ERROR) so maybe that's the more important criteria

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You know, now that I think about it a bit more, having the individual error lines will be more important when the hostname is also included on the line so we can separate out overlapping logs in multi-container deployments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So if we keep it as-is, I'll just need a ✅

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't help here if you want ERROR on every line, but I just wanted to note that log.exception('error message') formats and logs the traceback automatically without a need to call format_exception.

log.error(error_line)


def refresh_event_type_map() -> None:
Expand Down
Loading