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

Fix log subscriber unsubscriptions #275

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elia
Copy link

@elia elia commented Jan 23, 2019

There's a bug in ActionView#public_methods(false) that makes it not returning
subscription methods. Using #patterns the log subscribed is asked directly for
its supported event names.

This becomes evident when using Lograge with a lower log level where rendering
logs render-time for each template/partial.

Also added ActiveRecord to the mix that is very noisy when using lograge in development.

There's a bug in ActionView#public_methods(false) that makes it not returning
subscription methods. Using #patterns the log subscribed is asked directly for
its supported event names.

This becomes evident when using Lograge with a lower log level where rendering
logs render-time for each template/partial.
Using Lograge in development or with a lower log-level will make AR logs to be
printed.
@benlovell
Copy link
Collaborator

Do you have a reference the bug you mentioned?

@elia
Copy link
Author

elia commented Feb 8, 2019

Do you have a reference [to] the bug you mentioned?

Sorry for the delay, I tried to replicate on a fresh app that I could share, but I couldn't. Anyway the app where I'm observing the bug is ruby 2.3.7, rails 5.1, solidus 2.4 and the usual ton of gems.

I can say though that I noticed that the problem is wider, for example I'm seeing messages from ActiveModel::Serializer in production (see screenshot below). So the problem is more widespread and I think would be useful to have a general way of unsubscribing from noisy components, defaulting to the current environment.

image

NOTE: I'll wait for instructions on how to proceed before fixing the specs and/or revising the PR 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants