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

Subscribe to additional Action controller topics #1744

Merged
merged 40 commits into from
Jan 24, 2023

Conversation

tannalynn
Copy link
Contributor

@tannalynn tannalynn commented Jan 20, 2023

  • Added a start/finish methods to the parent class to share with other classes that are not doing anything complicated. I also removed the methods from subclasses that no longer need it.
  • I also moved the tests related to the start and finish methods into a notifications_subscriber test
  • Since the new action controller topics were so different from process_action, it made more sense to create a new subscriber class for it
  • I did not subscribe to start_processing because it was just creating duplicates of process_action

passing on all versions with latest commit
closes #1510
closes #1512

@@ -163,42 +159,6 @@ def test_pop_segment_returns_false
end
end

def test_start_logs_notification_error
Copy link
Contributor Author

@tannalynn tannalynn Jan 20, 2023

Choose a reason for hiding this comment

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

Moved these tests into the notifications_subscriber.rb since these are testing what is now shared code

@tannalynn tannalynn marked this pull request as ready for review January 20, 2023 20:21

def finish_segment(name, id, payload)
raise NotImplementedError
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great cleanup, thank you.

tannalynn and others added 4 commits January 20, 2023 13:11
Refactor ActionViewSubscriber methods `start` and `finish` (which now inherit from
NotificationsSubscriber to `start_segment` and `finish_segment`.
fallwith
fallwith previously approved these changes Jan 20, 2023
kaylareopelle
kaylareopelle previously approved these changes Jan 20, 2023
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thank you, Tanna! Great work!

kaylareopelle
kaylareopelle previously approved these changes Jan 24, 2023
fallwith
fallwith previously approved these changes Jan 24, 2023
fallwith
fallwith previously approved these changes Jan 24, 2023
@github-actions
Copy link
Contributor

SimpleCov Report

Coverage Threshold
Line 93.18% 93%
Branch 84.74% 84%

@@ -116,8 +114,6 @@ def test_super_operation_recorded_as_attribute_on_traces
trace = last_transaction_trace
tt_node = find_node_with_name(trace, "#{METRIC_PREFIX}#{DEFAULT_STORE}/read")

puts txn.segments.last.inspect
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for cleaning up these puts statements! Sorry I forgot to remove them!

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Great work, Tanna!

@tannalynn tannalynn merged commit 1d735e0 into dev Jan 24, 2023
@fallwith fallwith deleted the action_controller_subscriber_additional_topic branch January 25, 2023 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants