[Feature] Initial pass at removing qs logging in view of the PlayerEventLogging System #4542

neckkola · 2024-11-15T02:27:56Z

Description

In discussions with the devs, this is the start to migrate the qs logging system into the player event system. Given that the player event logging system uses a json payload, this will allow for that payload to be flatten into 'qs like' structures to allow for easier server operator queries.

At present, this enables:
aa purchase
killed npc, named and raid mobs
loot
merchant selling and purchasing
npc handins
player speech (off by default)
trades

The qs logging code for the above is still present. This can be removed (now or in the future) to allow for a migration path for operators.

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Testing

Tested locally on RoF2. Requires further testing and documentation on my part. I wanted to start the review process.

Clients tested:
N/A

Checklist

I have tested my changes
I have performed a self-review of my code. Ensuring variables, functions and methods are named in a human-readable way, comments are added only where naming of variables, functions and methods can't give enough context.
[TBD] I have made corresponding changes to the documentation (if applicable, if not delete this line)
I own the changes of my code and take responsibility for the potential issues that occur
If my changes make database schema changes, I have tested the changes on a local database (attach image). Updated version.h CURRENT_BINARY_DATABASE_VERSION to the new version. (Delete this if not applicable)

Add retention for etl tables Rename tables/fields to etl nomenclature Combine database work to one atomic load

testing passed though appears that the event itself has a few bugs. Will fix them in another commit

Cleanup and refactor a few items.

Cleanup and Formatting

Playerevent::Trade PlayerEvent::Speech (new event to mirror functionality of qs_speech

Playerevent::KilledNPC, KilledNamedNPC and KilledRaidNPC

Add etl for Playerevent::AA_purchase

Akkadius

Amazing work!

Some things we need to follow up on

All QS table code needs to be removed, period both in world and in QS ingestion. We will provide an optional transformer who wants to transform old tables to new data.
We will provide a data transformer from old QS tables to new. I can take this on.
Player event processing code needs to be on its own thread and its own database connection, I can also take this on
We need a command in world that exposes the ETL settings in JSON format so it can feed into Spire admin. Similar to ./bin/world database:schema
As you see in the PR comments, most of the vectors can be reserved on which does help substantially to bulk allocate memory once instead of doing it each event. We know the size of all of the data coming in, so we just need to reserve again even if it means doing another loop prior to fetch each event, call reserve in a switch.

Akkadius · 2024-11-16T02:53:50Z

common/events/player_event_logs.cpp

+							out_entries.augment_5_id = augments[4];
+							out_entries.augment_6_id = augments[5];
+						}
+					etl_queues.npc_handin_entries.push_back(out_entries);


Since we know the size of etl_queues.npc_handin_entries from in.handin_items we can do a reserve before the loop to pre-allocate memory

While we know the 'entries' sizes, there could be other events of the same type in the batch queue, so we would need to cycle through the batch_queue to parse the total size for the potential multiple events. Would this be beneficial, instead of the dynamic size adjustments as the vectors are add to? If so, happy to do that.
Same comment for all the reserve items.

Yeah it makes a huge difference to pre-allocate especially with higher event volume.

Added reserve to all etl_queues.

Thank you sir

common/events/player_event_logs.cpp

Akkadius · 2024-11-16T04:21:48Z

common/events/player_event_logs.cpp

@@ -123,21 +134,466 @@ void PlayerEventLogs::ProcessBatchQueue()

 	BenchTimer benchmark;

+	EtlQueues etl_queues{};


Looking at all of this in ProcessBatchQueue and in our deletions, I think we should wrap these functions in thread calls so we don't tie up the main thread.

This also means that we should create a separate database connection just for processing player events because what happens is even if we had all of this in another thread its going to be lock contended with database locks on the main thread.

World needs to be kept as lightweight as possible in the main thread and this certainly adds weight if anything starts to take more than 500ms which it most certainly could here.

Same would go for using QS if folks use QS to process.

I may end up taking this one on.

common/events/player_event_logs.cpp

zone/client.cpp

zone/client_packet.cpp

zone/client.cpp

Correct a failed test case for improper next id for etl tables when table is first created.

…nts.

…his.

Akkadius · 2024-11-18T05:16:24Z

common/repositories/player_event_speech_repository.h

+	{
+		auto results = db.QueryDatabase(
+			fmt::format(
+				"SELECT AUTO_INCREMENT FROM information_schema.tables WHERE TABLE_NAME = '{}';",


We should assume that the server user can't directly query information_schema is there a reason we leaned into this versus max + 1 ?

Yes. I ran into a couple of test cases that failed. If I empty an etl table, the max + 1 will return 1, when in fact the auto_increment may be 100. Further, once the retention period is hit, the next id via max + 1 will again be different from the auto_increment.

Akkadius · 2024-11-18T05:18:46Z

world/main.cpp

@@ -441,7 +441,7 @@ int main(int argc, char **argv)
 		}

 		if (player_event_process_timer.Check()) {
-			player_event_logs.Process();
+			std::jthread event_thread(&PlayerEventLogs::Process, &player_event_logs);


There's also a spot in queryserv

Akkadius · 2024-11-18T05:20:07Z

Amazing work sir!

One more stretch goal if you are down for it -

Have zoneservers directly connect to queryserv if it is available so that events go straight to queryserv instead of going through world

neckkola added 21 commits November 14, 2024 21:53

First pass of player_event_loot_items

ece6eee

Second pass of player_event_loot_items

36b499c

Third pass of player_event_loot_items

f710ada

Example without RecordDetailEvent template

04298e3

Cleanup the removal of the template

d74a4bd

Fourth Pass

130bf2f

Add retention for etl tables Rename tables/fields to etl nomenclature Combine database work to one atomic load

Reposition to reduce db tasks

2b05a12

Refactor etl processing for easier additions

e3c99a1

Add merchant purchase event

436f280

testing passed though appears that the event itself has a few bugs. Will fix them in another commit

Fix PlayerEventMerchantPurchase in client_packet.cpp

11a2c56

WIP - Handin

7efcaa2

Handin Event added

dd5093f

Cleanup

534a9a0

All a rentention period of 0 days which deletes all current records.

3f4755e

Updates

57825ed

Cleanup and refactor a few items.

Cleanup and Formatting

134e408

Cleanup and Formatting

Add etl for

d6c1022

Playerevent::Trade PlayerEvent::Speech (new event to mirror functionality of qs_speech

Add etl for

75f89fe

Playerevent::KilledNPC, KilledNamedNPC and KilledRaidNPC

Add etl for Playerevent::AA_purchase

ab82afa

Add etl for Playerevent::AA_purchase

Set etl for speech to off by default

cfc777b

Cleanup before PR

6605809

Akkadius reviewed Nov 16, 2024

View reviewed changes

zone/client_packet.cpp Show resolved Hide resolved

Akkadius reviewed Nov 16, 2024

View reviewed changes

zone/client.cpp Show resolved Hide resolved

neckkola added 5 commits November 16, 2024 19:51

Review comment updates.

7ca379d

Add world cli etl:settings to output a json on all player event details.

8f69f73

Add reserve for all etl_queues

8e4aaee

Correct a failed test case for improper next id for etl tables when table is first created.

Potential solution for a dedicated database connection for player eve…

ab7f8d5

…nts.

Simple thread for player_events. Likely there is a better way to do t…

78a538e

…his.

Akkadius reviewed Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Initial pass at removing qs logging in view of the PlayerEventLogging System #4542

[Feature] Initial pass at removing qs logging in view of the PlayerEventLogging System #4542

neckkola commented Nov 15, 2024

Akkadius left a comment •

edited by neckkola

Loading

Akkadius Nov 16, 2024

neckkola Nov 16, 2024

Akkadius Nov 17, 2024 •

edited

Loading

neckkola Nov 17, 2024

Akkadius Nov 18, 2024

Akkadius Nov 16, 2024

Akkadius Nov 18, 2024

neckkola Nov 18, 2024

Akkadius Nov 18, 2024

Akkadius commented Nov 18, 2024

		@@ -123,21 +134,466 @@ void PlayerEventLogs::ProcessBatchQueue()

		BenchTimer benchmark;

		EtlQueues etl_queues{};

[Feature] Initial pass at removing qs logging in view of the PlayerEventLogging System #4542

Are you sure you want to change the base?

[Feature] Initial pass at removing qs logging in view of the PlayerEventLogging System #4542

Conversation

neckkola commented Nov 15, 2024

Description

Type of change

Testing

Checklist

Akkadius left a comment • edited by neckkola Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akkadius Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akkadius commented Nov 18, 2024

Akkadius left a comment •

edited by neckkola

Loading

Akkadius Nov 17, 2024 •

edited

Loading