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

Remove child event tracking from ActiveSupport::Subscriber #43390

Merged
merged 3 commits into from
Feb 17, 2022

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Oct 6, 2021

Previously ActiveSupport::Subscriber would manage its own events, keep them in a thread-local stack, and track "child" events of the same subscriber.

I don't think this was particularly useful, since it only considered notifications delivered to the same subscriber, and I can't find evidence of this being used in the wild. I think this was intended to support tracing, but any such tool would want to do this itself (a minimum in order to support multiple namespaces).

Additionally, this has caused a few users to OOM in production environments, notably when queueing many jobs from another job. Based on the limited use and significant issues, I'd like to remove this tracking.

Based on my software archaeology this was introduced to solve issues seen in the newrelic agent #5932. The modern agent doesn't use this as far as I can tell (instead it builds and keeps track of its own events based on start/stop). IMO this implementation wouldn't be useful for it for the reasons described above.

In the second commit I remove children and parent_of?. My thinking is that in the off chance anyone is using this it should error loudly. However if preferred I could return sensible default values and emit a deprecation warning.

Anyone who needs this functionality can (and likely wants to) re-implement it in their own start/stop based subscription.

Fixes #21036 cc @pixeltrix (there's some discussion about nested tags for logging, which this change keeps intact 🙂)

jhawthorn and others added 3 commits February 17, 2022 08:20
Previously ActiveSupport::Subscriber would manage its own events, keep
them in a thread-local stack, and track "child" events of the same
subscriber.

I don't think this was particularly useful, since it only considered
notifications delivered to the same subscriber, and I can't find
evidence of this being used in the wild. I think this was intended to
support tracing, but any uch tool would want to do this itself (at
minimum in order to support multiple namespaces).

Additionally, this has caused a few users to OOM in production
environments, notably when queueing many jobs from another job.
Previously one subscriber was used for both the "Rendering" (before) and
"Rendered" (after) events. With the previous change to AS::Subscriber
these need to be split.

Co-authored-by: Adam Hess <HParker@github.com>
@jhawthorn jhawthorn force-pushed the remove_notification_event_children branch from 30467a9 to 9c58a54 Compare February 17, 2022 16:20
@jhawthorn jhawthorn merged commit 3e2f9a6 into rails:main Feb 17, 2022
yykamei added a commit to yykamei/rails_band that referenced this pull request Apr 4, 2022
`#children` has been deprecated as of Rails 7.2.

rails/rails#43390

Unlike Rails, I decided to remove `#children` from `BaseEvent`
since it'd be a techdebt to keep this method in the future,
and I guess this method would not be used very well.

By the way, I found this deprecation on Rails when I'm about to
add the support for `deprecation.rails` on #89. I hope this won't cause
problems for users who use this gem.
yykamei added a commit to yykamei/rails_band that referenced this pull request Apr 4, 2022
`#children` has been deprecated as of Rails 7.2.

rails/rails#43390

Unlike Rails, I decided to remove `#children` from `BaseEvent`
since it'd be a techdebt to keep this method in the future,
and I guess this method would not be used very well.

By the way, I found this deprecation on Rails when I'm about to
add the support for `deprecation.rails` on #89. I hope this won't cause
problems for users who use this gem.
@DougEdey
Copy link

DougEdey commented Nov 6, 2023

👋 I know it's not going to change anything, but event though this is marked as deprecated in 7.1, it's actually removed because the children access is removed, and the #children method returns an empty array

stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Apr 23, 2024
The [introduction][] of `config/initializers/active_job.rb` was rendered
obsolete by [rails/rails#43390][].

[introduction]: 38b530c
[rails/rails#43390]: rails/rails#43390
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Apr 23, 2024
The [introduction][] of `config/initializers/active_job.rb` was rendered
obsolete by [rails/rails#43390][].

Additionally, the following Rails 7 defaults match the [existing
configuration][], so there is no need to redeclare them.

```ruby
Rails.application.config.action_mailer.deliver_later_queue_name
=> nil

Rails.application.config.action_mailbox.queues.routing
=> nil

Rails.application.config.active_storage.queues.analysis
=> nil

Rails.application.config.active_storage.queues.purge
=> nil

Rails.application.config.active_storage.queues.mirror
=> nil
```

This is relevant because the next release of Suspenders will only
support `rails >= 7.0`.

[introduction]: 38b530c
[rails/rails#43390]: rails/rails#43390
[existing configuration]: https://github.com/thoughtbot/suspenders/blob/bd40e33a585891afba380a7884284f5accc003cf/lib/suspenders/generators/jobs_generator.rb#L19-L23
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Apr 23, 2024
The [introduction][] of `config/initializers/active_job.rb` was rendered
obsolete by [rails/rails#43390][].

Additionally, the following Rails 7 defaults match the [existing
configuration][], so there is no need to redeclare them.

```ruby
Rails.application.config.action_mailer.deliver_later_queue_name
=> nil

Rails.application.config.action_mailbox.queues.routing
=> nil

Rails.application.config.active_storage.queues.analysis
=> nil

Rails.application.config.active_storage.queues.purge
=> nil

Rails.application.config.active_storage.queues.mirror
=> nil
```

This is relevant because the next release of Suspenders will only
support `rails >= 7.0`.

[introduction]: 38b530c
[rails/rails#43390]: rails/rails#43390
[existing configuration]: https://github.com/thoughtbot/suspenders/blob/bd40e33a585891afba380a7884284f5accc003cf/lib/suspenders/generators/jobs_generator.rb#L19-L23
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Apr 23, 2024
The [introduction][] of `config/initializers/active_job.rb` was rendered
obsolete by [rails/rails#43390][].

Additionally, the following Rails 7 defaults match the [existing
configuration][], so there is no need to redeclare them.

```ruby
Rails.application.config.action_mailer.deliver_later_queue_name
=> nil

Rails.application.config.action_mailbox.queues.routing
=> nil

Rails.application.config.active_storage.queues.analysis
=> nil

Rails.application.config.active_storage.queues.purge
=> nil

Rails.application.config.active_storage.queues.mirror
=> nil
```

This is relevant because the next release of Suspenders will only
support `rails >= 7.0`.

[introduction]: 38b530c
[rails/rails#43390]: rails/rails#43390
[existing configuration]: https://github.com/thoughtbot/suspenders/blob/bd40e33a585891afba380a7884284f5accc003cf/lib/suspenders/generators/jobs_generator.rb#L19-L23
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Apr 24, 2024
Follow-up to #1147

The [introduction][] of `config/initializers/active_job.rb` was rendered
obsolete by [rails/rails#43390][].

Additionally, the following Rails 7 defaults match the [existing
configuration][], so there is no need to redeclare them.

```ruby
Rails.application.config.action_mailer.deliver_later_queue_name
=> nil

Rails.application.config.action_mailbox.queues.routing
=> nil

Rails.application.config.active_storage.queues.analysis
=> nil

Rails.application.config.active_storage.queues.purge
=> nil

Rails.application.config.active_storage.queues.mirror
=> nil
```

This is relevant because the next release of Suspenders will only
support `rails >= 7.0`.

[introduction]: 38b530c
[rails/rails#43390]: rails/rails#43390
[existing configuration]: https://github.com/thoughtbot/suspenders/blob/bd40e33a585891afba380a7884284f5accc003cf/lib/suspenders/generators/jobs_generator.rb#L19-L23
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request May 10, 2024
Follow-up to #1147

The [introduction][] of `config/initializers/active_job.rb` was rendered
obsolete by [rails/rails#43390][].

Additionally, the following Rails 7 defaults match the [existing
configuration][], so there is no need to redeclare them.

```ruby
Rails.application.config.action_mailer.deliver_later_queue_name
=> nil

Rails.application.config.action_mailbox.queues.routing
=> nil

Rails.application.config.active_storage.queues.analysis
=> nil

Rails.application.config.active_storage.queues.purge
=> nil

Rails.application.config.active_storage.queues.mirror
=> nil
```

This is relevant because the next release of Suspenders will only
support `rails >= 7.0`.

[introduction]: 38b530c
[rails/rails#43390]: rails/rails#43390
[existing configuration]: https://github.com/thoughtbot/suspenders/blob/bd40e33a585891afba380a7884284f5accc003cf/lib/suspenders/generators/jobs_generator.rb#L19-L23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enqueuing jobs inside a job can lead to memory bloat
2 participants