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.

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

Ensure error logs are always printed #822

Merged
merged 8 commits into from
Jun 11, 2024
Merged

Conversation

FelonEkonom
Copy link
Member

No description provided.

@FelonEkonom FelonEkonom self-assigned this Jun 10, 2024
@FelonEkonom FelonEkonom requested a review from mat-hek as a code owner June 10, 2024 14:49
@FelonEkonom FelonEkonom added the no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md label Jun 10, 2024
@FelonEkonom FelonEkonom requested a review from varsill June 10, 2024 14:50
@@ -0,0 +1,24 @@
defmodule Membrane.Core.Macros do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defmodule Membrane.Core.Macros do
defmodule Membrane.Core.Utils do

Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

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

How about adding it to other GenServers that we define in Core as well (like ResourceGuard etc.?)

Comment on lines 212 to 214
Macros.log_on_error do
do_handle_info(message, state)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

why Telemetry.report_metric is outside of the Macros.log_on_error?

lib/membrane/core/macros.ex Outdated Show resolved Hide resolved
@FelonEkonom FelonEkonom requested a review from varsill June 11, 2024 10:00
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

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

Initially I just thought of adding it only in other processes that can execute users code (just like ResourceGuard, which executes registered user's lambda), but since you have added logging to all GenServers it's even better, as it will be easier for us to debug if we made some mistakes :D

@FelonEkonom FelonEkonom merged commit 5f53b95 into master Jun 11, 2024
5 of 6 checks passed
@FelonEkonom FelonEkonom deleted the ensure-errors-logged branch June 11, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants