search

Team-Silver-Sphere / SquadJS

Squad Server Script Framework
Boost Software License 1.0
167 stars 127 forks source link

feat(db_log): do not save duplicated values for tickRate and PlayerCount models #390

Closed anklav24 closed 1 week ago

werewolfboy13 commented 2 weeks ago

Reasoning behind this change?

I am seeing the files changed beyond the description.

werewolfboy13 commented 2 weeks ago

This PR appears to be breaking.

anklav24 commented 2 weeks ago

Needs more information.

Sorry, I put the wrong commit the first time. Check it out now.

anklav24 commented 2 weeks ago

@werewolfboy13 There is no need to save values on each tick if they are repeated; this greatly increase the database over time. I have 2 public and 4 custom servers. Even with this optimization, I currently have 4,456,464 records in the database.

werewolfboy13 commented 2 weeks ago

This one I'll take a peek. There is some grafana dashboards that rely on that. This is something I have to take into consideration.

I tend to look over all the PR's once I am done working.

anklav24 commented 2 weeks ago

Screenshot 2024-11-08 111610

Grafana can easily handle such gaps.

steelskillet commented 2 weeks ago

This table shows the difference between recording every tick and only non-duplicated values (for tick rate). There's really not that big of difference between the two, while it may seem like a lot, due to how indexing and database optimizations work (in big O, its log(n)), there's not that big of a difference. if you need better performance in your DB you should look into indexing, partitions and possibly a better RDBMS or (very unlikely) better hardware. space savings wise, this probably amounts to just a few MB, which is about or less than 1% of the DB storage size. image

anklav24 commented 2 weeks ago

This table shows the difference between recording every tick and only non-duplicated values (for tick rate). There's really not that big of difference between the two, while it may seem like a lot, due to how indexing and database optimizations work (in big O, its log(n)), there's not that big of a difference. if you need better performance in your DB you should look into indexing, partitions and possibly a better RDBMS or (very unlikely) better hardware. space savings wise, this probably amounts to just a few MB, which is about or less than 1% of the DB storage size. image

Why store any unnecessary data when you can store none? 😀

werewolfboy13 commented 2 weeks ago

I'm with @steelskillet there is no point for this PR.

steelskillet commented 2 weeks ago

Just to clarify, the con of changing the behaviour of how the data is saved outweighs any benefit this change has. Obviously saving only the changes is better and would be good for new software but squadjs isn't new software.

anklav24 commented 2 weeks ago

Just to clarify, the con of changing the behaviour of how the data is saved outweighs any benefit this change has. Obviously saving only the changes is better and would be good for new software but squadjs isn't new software.

Could you please elaborate on the cons?

steelskillet commented 2 weeks ago

The con is changing the behaviour. People build tools and plugins that rely on and are based on the current behaviour of the program, changing that could mess those tools and plugins up. Eg right now you can use the steady tickrate entries to deduce a keep alive signal, at least while players are on. There can also be weird and obscure uses that we may not know about that relies on the current function of that method or even structure of the plugin, in JavaScript you can rewrite functions during runtime and initialization. While these are normally miniscule in comparison to the pros of a change, eg new features or better performance or bug fixes, when the pro of the change is also miniscule (performance improvement within the margin of error), keeping the status quo of the current program is much more desirable.

TLDR: the data you deem is unnecessary could be mission critical to another person while showing no noticable improvement for anybody if removed.

anklav24 commented 2 weeks ago

TLDR: the data you deem is unnecessary could be mission critical to another person while showing no noticable improvement for anybody if removed.

As far as I understand, SquadJS uses Convertional Commits system. This means that those who rely on the current state of the program use a specific version and can see from it whether there have been any breaking changes.

steelskillet commented 2 weeks ago

As far as I can tell, this change offers almost nothing in way of performance, why break what others are currently doing for nothing? What if their system relies on the data you think is unnecessary? Why make that person and others like them do development work to undo something that offers almost no benefits? How will they maintain their work when other updates are layered on this one? What about tools that access the database directly and have no real relation directly to squadjs? That means those people have to maintain their own version of squadjs just so their tool still works for practically no reason. Do you plan to just leave these people in the dust for future commits and features all for some change that has no noticeable improvement? This change would remove data getting recorded, if there are people who rely on this data, there is no way for them to get this without just straight up undoing what this change does. That is a large con compared to a negligible performance improvement.

werewolfboy13 commented 1 week ago

Closing due to this PR not needing to be pushed to prod. Possible uses of current system out-way the benefits of this PR.