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

Denormalize event type for request and event logs #829

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
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
Prev Previous commit
denormalize event type for request and event logs
  • Loading branch information
ezekg committed May 1, 2024
commit 5dde81538f833bb334f962bc30cba0e3efe425e6
6 changes: 3 additions & 3 deletions app/models/concerns/denormalizable.rb
Original file line number Diff line number Diff line change
@@ -49,7 +49,7 @@ def instrument_denormalized_attribute_from(attribute_name, from:, prefix:)

after_initialize -> { write_denormalized_attribute_from_schrodingers_record(association_name, attribute_name, prefixed_attribute_name) }, if: -> { send(:"#{reflection.foreign_key}_changed?") || send(:"#{reflection.name}_changed?") }, unless: :persisted?
before_validation -> { write_denormalized_attribute_from_schrodingers_record(association_name, attribute_name, prefixed_attribute_name) }, if: -> { send(:"#{reflection.foreign_key}_changed?") || send(:"#{reflection.name}_changed?") }, on: :create
before_update -> { write_denormalized_attribute_from_persisted_record(association_name, attribute_name, prefixed_attribute_name) }, if: -> { send(:"#{reflection.foreign_key}_changed?") || send(:"#{reflection.name}_changed?") }
before_update -> { write_denormalized_attribute_from_persisted_record(association_name, attribute_name, prefixed_attribute_name) }, if: -> { send(:"#{reflection.foreign_key}_changed?") || send(:"#{reflection.name}_changed?") }

# Make sure validation fails if our denormalized column is modified directly
validate -> { validate_denormalized_attribute_from_persisted_record(association_name, attribute_name, prefixed_attribute_name) }, if: :"#{prefixed_attribute_name}_changed?", on: :update
@@ -75,11 +75,11 @@ def instrument_denormalized_attribute_to(attribute_name, to:, prefix:)
if reflection.collection?
after_initialize -> { write_denormalized_attribute_to_unpersisted_relation(association_name, prefixed_attribute_name, attribute_name) }, if: :"#{attribute_name}_changed?", unless: :persisted?
before_validation -> { write_denormalized_attribute_to_unpersisted_relation(association_name, prefixed_attribute_name, attribute_name) }, if: :"#{attribute_name}_changed?", on: :create
after_update -> { write_denormalized_attribute_to_persisted_relation(association_name, prefixed_attribute_name, attribute_name) }, if: :"#{attribute_name}_previously_changed?"
after_save -> { write_denormalized_attribute_to_persisted_relation(association_name, prefixed_attribute_name, attribute_name) }, if: :"#{attribute_name}_previously_changed?"
else
after_initialize -> { write_denormalized_attribute_to_unpersisted_record(association_name, prefixed_attribute_name, attribute_name) }, if: :"#{attribute_name}_changed?", unless: :persisted?
before_validation -> { write_denormalized_attribute_to_unpersisted_record(association_name, prefixed_attribute_name, attribute_name) }, if: :"#{attribute_name}_changed?", on: :create
after_update -> { write_denormalized_attribute_to_persisted_record(association_name, prefixed_attribute_name, attribute_name) }, if: :"#{attribute_name}_previously_changed?"
after_save -> { write_denormalized_attribute_to_persisted_record(association_name, prefixed_attribute_name, attribute_name) }, if: :"#{attribute_name}_previously_changed?"
end
else
raise ArgumentError, "invalid :to association: #{to.inspect}"
4 changes: 4 additions & 0 deletions app/models/event_log.rb
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@

class EventLog < ApplicationRecord
include Keygen::EE::ProtectedClass[entitlements: %i[event_logs]]
include Denormalizable
include Environmental
include Accountable
include DateRangeable
@@ -22,6 +23,9 @@ class EventLog < ApplicationRecord
has_environment
has_account

denormalizes :event, from: :event_type, prefix: :event_type
denormalizes :event_type_id, to: :request_log
Copy link
Member Author

@ezekg ezekg Jan 13, 2025

Choose a reason for hiding this comment

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

Possible race condition here since there's no guarantee an event log will be inserted after a request log. Need to rethink this (and write appropriate tests).


# NOTE(ezekg) Would love to add a default instead of this, but alas,
# the table is too big and it would break everything.
before_create -> { self.created_date ||= (created_at || Date.current) }
4 changes: 4 additions & 0 deletions app/models/request_log.rb
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@

class RequestLog < ApplicationRecord
include Keygen::EE::ProtectedClass[entitlements: %i[request_logs]]
include Denormalizable
include Environmental
include Accountable
include DateRangeable
@@ -11,12 +12,15 @@ class RequestLog < ApplicationRecord

belongs_to :requestor, polymorphic: true, optional: true
belongs_to :resource, polymorphic: true, optional: true
belongs_to :event_type, optional: true
Copy link
Member Author

Choose a reason for hiding this comment

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

Where is this being set?

has_one :event_log,
inverse_of: :request_log

has_environment
has_account

denormalizes :event, from: :event_type, prefix: :event_type
Copy link
Member Author

@ezekg ezekg Jan 13, 2025

Choose a reason for hiding this comment

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

Should we be denormalizing event_type_id from event_log too? Would solve for the race condition touched on here.


# NOTE(ezekg) Would love to add a default instead of this, but alas,
# the table is too big and it would break everything.
before_create -> { self.created_date ||= (created_at || Date.current) }
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class AddEventTypeEventToEventLogs < ActiveRecord::Migration[7.1]
verbose!

def change
add_column :event_logs, :event_type_event, :string, null: true
ezekg marked this conversation as resolved.
Show resolved Hide resolved
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class AddEventTypeEventToRequestLogs < ActiveRecord::Migration[7.1]
verbose!

def change
add_column :request_logs, :event_type_event, :string, null: true
add_column :request_logs, :event_type_id, :uuid, null: true
ezekg marked this conversation as resolved.
Show resolved Hide resolved
end
end
5 changes: 4 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.1].define(version: 2024_04_24_041244) do
ActiveRecord::Schema[7.1].define(version: 2024_05_01_124926) do
# These are extensions that must be enabled in order to support this database
enable_extension "btree_gin"
enable_extension "pg_stat_statements"
@@ -110,6 +110,7 @@
t.datetime "updated_at", null: false
t.uuid "environment_id"
t.date "created_date"
t.string "event_type_event"
t.index ["account_id", "created_at"], name: "index_event_logs_on_account_id_and_created_at", order: { created_at: :desc }
t.index ["account_id", "created_date"], name: "index_event_logs_on_account_id_and_created_date", order: { created_date: :desc }
t.index ["environment_id"], name: "index_event_logs_on_environment_id"
@@ -695,6 +696,8 @@
t.jsonb "response_headers"
t.float "run_time"
t.float "queue_time"
t.string "event_type_event"
t.uuid "event_type_id"
t.index ["account_id", "created_at"], name: "index_request_logs_on_account_id_and_created_at"
t.index ["account_id", "created_date"], name: "index_request_logs_on_account_id_and_created_date", order: { created_date: :desc }
t.index ["environment_id"], name: "index_request_logs_on_environment_id"
64 changes: 64 additions & 0 deletions spec/models/event_log_spec.rb
Original file line number Diff line number Diff line change
@@ -4,6 +4,70 @@
require 'spec_helper'

describe EventLog, type: :model do
let(:account) { create(:account) }

it_behaves_like :environmental
it_behaves_like :accountable

describe '#event_type=' do
let(:event_type) { create(:event_type) }

context 'on build' do
it 'should denormalize event from event type' do
event_log = build(:event_log, event_type:, account:)

expect(event_log.event_type_event).to eq event_type.event
expect(event_log.event_type_id).to eq event_type.id
end

it 'should denormalize event type to request log' do
request_log = build(:request_log, account:)
event_log = build(:event_log, request_log:, event_type:, account:)

expect(request_log.event_type_event).to be_nil
expect(request_log.event_type_id).to eq event_type.id
end
end

context 'on create' do
it 'should denormalize event from event type' do
event_log = create(:event_log, event_type:, account:)

expect(event_log.event_type_event).to eq event_type.event
expect(event_log.event_type_id).to eq event_type.id
end

it 'should denormalize event type to request log' do
request_log = create(:request_log, account:)
event_log = create(:event_log, request_log:, event_type:, account:)

request_log.reload

expect(request_log.event_type_event).to eq event_type.event
expect(request_log.event_type_id).to eq event_type.id
end
end

context 'on update' do
it 'should denormalize event from event type' do
event_log = create(:event_log, account:)

event_log.update!(event_type:)

expect(event_log.event_type_event).to eq event_type.event
expect(event_log.event_type_id).to eq event_type.id
end

it 'should denormalize event type to request log' do
request_log = create(:request_log, account:)
event_log = create(:event_log, request_log:, account:)

event_log.update!(event_type:)
request_log.reload

expect(request_log.event_type_event).to eq event_type.event
expect(request_log.event_type_id).to eq event_type.id
end
end
end
end
32 changes: 32 additions & 0 deletions spec/models/request_log_spec.rb
Original file line number Diff line number Diff line change
@@ -4,6 +4,38 @@
require 'spec_helper'

describe RequestLog, type: :model do
let(:account) { create(:account) }

it_behaves_like :environmental
it_behaves_like :accountable

describe '#event_type=' do
let(:event_type) { create(:event_type) }

context 'on build' do
it 'should denormalize event from event type' do
request_log = build(:request_log, event_type:, account:)

expect(request_log.event_type_event).to eq event_type.event
end
end

context 'on create' do
it 'should denormalize event from event type' do
request_log = create(:request_log, event_type:, account:)

expect(request_log.event_type_event).to eq event_type.event
end
end

context 'on update' do
it 'should denormalize event from event type' do
request_log = create(:request_log, account:)

request_log.update!(event_type:)

expect(request_log.event_type_event).to eq event_type.event
end
end
end
end