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

Added log to verify that method is getting called at right condition #1081

Merged
merged 1 commit into from
Jan 8, 2024
Merged

Added log to verify that method is getting called at right condition #1081

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
5 changes: 4 additions & 1 deletion app/models/concerns/indexable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ module Indexable
if aasm_state == "findable"
changed_attributes = saved_changes
relevant_changes = changed_attributes.keys & %w[related_identifiers creators funding_references aasm_state]
send_import_message(to_jsonapi) if relevant_changes.any?
if relevant_changes.any?
send_import_message(to_jsonapi)
Rails.logger.info "[Event Data Import Message] #{aasm_state} #{to_jsonapi.inspect} send to Event Data service."
Copy link
Contributor

Choose a reason for hiding this comment

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

This will potentially create a lot of log entries in production for every update that is triggering a import.
You could also get this on the other side during the actual import, i.e. levriero.
I think this is fine if you really need this information to debug in production, but if you can check how many logs are being added that'd be good, and if it's a lot (discuss on slack) then we might want to remove it at some point. Log collection does have an associated cost as well as being extra data to parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @richardhallett I will not be pushing this to production. I just want to test this on staging. I will be removing this log once testing is done. Sorry I did not mentioned this in the description

end
end
elsif instance_of?(Event)
OtherDoiJob.perform_later(dois_to_import)
Expand Down