query command overhaul #29

mightyBroccoli · 2019-10-17T01:47:12Z

cleared up serverinfo and hostinfo mixup

some parameters where attached to the wrong argument eg. connection_packets_* parameter, which are not part of the hostinfo output

instanceinfo

add ServerQueryMaxConnectionsPerIP

hostinfo

add InstanceUptime
add HostTimestamp
add VirtualServersRunning

serverinfo

order parameters

add connection_packets parameter to serverinfo group
add connection_bytes parameter to serverinfo group

add new mockserver values

cleared up serverinfo and hostinfo mixup some parameters where attached to the wrong argument eg. connection_packets_* parameter, which are not part of the hostinfo output instanceinfo + add ServerQueryMaxConnectionsPerIP hostinfo + add InstanceUptime + add HostTimestamp + add VirtualServersRunning serverinfo * order parameters + add connection_packets parameter to serverinfo group + add connection_bytes parameter to serverinfo group * add new mockserver values

mightyBroccoli · 2019-10-17T18:01:08Z

I really dont know why the first test results in
server_cmds.go:62:13: struct of size 880 bytes could be of size 856 bytes (maligned)

--edit
I can confirm that this pull request fully works and that the new field finally yield data :)

stevenh · 2019-10-18T07:40:27Z

Your issue is the ordering of the fields, which means the struct is taking more space than it should.

This is caused by small types such as float32, bool etc.

You can download my pr from tyranron/golang-sizeof.tips#7 and run it locally which will allow you to visualise the issue.

* change bool to int serverinfo does not provide any boolean info

stevenh

Switching the type is the wrong thing, see my detailed comment on the commit.

mightyBroccoli · 2019-10-18T13:16:45Z

My last commit wasn't actually meant to fix the issue but fix an inherent issue. Those types I changed aren't actually booleans but integers.
Fixing the ordering issue is my next todo on the list.

* alignment fixup

mightyBroccoli · 2020-02-12T10:08:23Z

Would someone mind to review this?

mightyBroccoli · 2020-02-26T16:12:48Z

@stevenh would you mind to review my changes?

stevenh

First off apologies for the delay in reviewing this, is saw the original notification but then forgot, I enabled the new slack reminders this week and it just popped up; so hopefully that will help me not loose them in the future :)

Anyway here's some feedback

server_cmds.go

+ add proper gitignore file

* update mockserver command output to better represent the latest server version * changed any uptime variable to use time.duration * update server_cmds_test file to represent the changes done to server_cmds.go

* fix naming inconsistancy

* ajust port types to uint16

* ajust "total" values to use uint16 instead of int * ajust timestamps to use uint64 to give some headroom

server_cmds.go

server_cmds_test.go

.gitignore

stevenh

Miss clicked, changes required.

Co-Authored-By: Steven Hartland <[email protected]>

* update code commentary to feature proper sentences

* change bytes downloaded/uploaded values to uint64

* change flags to use boolean reponses

* ajusted values that return a timestamp to prase as a timestamp * replaced the serverrequestconnectioninfo with hostinfo as the correct request in that place serverrequestconnectioninfo does not feature values like the hosttimestamp or virtualservers_running * ajusted the testcode to now feature hostinfo calls and not serverrequestconnectioninfo calls

mightyBroccoli · 2020-04-03T21:22:48Z

I just noticed that the serverrequestconnectioninfo call was the incorrect call to retrieve the listed values. throughout the code the call hostinfo is used but not actually used.

* change ServerNickname to Nickname * change LogPermissions to boolean * fixed LogQuery to actually use virtualserver_log_query

stevenh · 2020-04-06T11:32:39Z

server_cmds.go

+	MaxClients                             uint16        `ms:"virtualserver_maxclients"`
+	ClientsOnline                          uint16        `ms:"virtualserver_clientsonline"`
+	ChannelsOnline                         uint16        `ms:"virtualserver_channelsonline"`
+	Uptime                                 time.Duration `ms:"virtualserver_uptime"`


I don't see any custom unmarshaling or fix up so I think this will have the wrong value as uptime is reported in seconds not nano seconds, same for the other time.Duration fields.

I am fairly new to Go, so would you mind to point me into a direction to fix this?

I am unsure if this method is "the" correct way to do this and I would love some feedback on this.
The good news is, it does work correctly.

This is a good question and truth be told there's no perfect way of doing it for a json unmarshal, but a good article which goes over the options is here:
https://blog.gopheracademy.com/advent-2016/advanced-encoding-decoding/

For ours however we're using mapstructure which has a custom decoding support.

I think we could extend the hook which is here:
https://github.com/multiplay/go-ts3/blob/master/helpers.go#L150

server_cmds.go

server_cmds_test.go

stevenh · 2020-04-06T11:35:27Z

server_cmds_test.go

 			AntiFloodPointsNeededCommandBlock:      150,
 			AntiFloodPointsNeededIPBlock:           250,
 			AntiFloodPointsTickReduce:              5,
 			ComplainAutoBanCount:                   5,
 			ComplainAutoBanTime:                    1200,
 			ComplainRemoveTime:                     3600,
+			Created:                                time.Unix(2, 0),


This looks wrong, I very much doubt it was created in 1970's

aren't those values just for testing purposes only? I only increased the value to 2 to mitigate some weird boolean / integer mixup.

The values where real values from a real request, then confirmed to be the correct conversion where needed.

This helps avoid broken made up data.

server_cmds_test.go

* partly reverse 0d3d6262590d0 serverrequestconnectioninfo was actually the correct call but the array I assign wasn't this fixes the array and correcty my rookie mistake

+ add struct function to convert time.duration nanoseconds to seconds

stevenh · 2020-04-06T21:38:04Z

server_cmds_test.go

 			AntiFloodPointsNeededCommandBlock:      150,
 			AntiFloodPointsNeededIPBlock:           250,
 			AntiFloodPointsTickReduce:              5,
 			ComplainAutoBanCount:                   5,
 			ComplainAutoBanTime:                    1200,
 			ComplainRemoveTime:                     3600,
+			Created:                                time.Unix(2, 0),


The values where real values from a real request, then confirmed to be the correct conversion where needed.

This helps avoid broken made up data.

stevenh · 2020-04-06T21:49:07Z

server_cmds.go

+	MaxClients                             uint16        `ms:"virtualserver_maxclients"`
+	ClientsOnline                          uint16        `ms:"virtualserver_clientsonline"`
+	ChannelsOnline                         uint16        `ms:"virtualserver_channelsonline"`
+	Uptime                                 time.Duration `ms:"virtualserver_uptime"`


This is a good question and truth be told there's no perfect way of doing it for a json unmarshal, but a good article which goes over the options is here:
https://blog.gopheracademy.com/advent-2016/advanced-encoding-decoding/

For ours however we're using mapstructure which has a custom decoding support.

I think we could extend the hook which is here:
https://github.com/multiplay/go-ts3/blob/master/helpers.go#L150

stevenh · 2020-04-06T21:58:09Z

server_cmds.go

+	BandwidthReceivedLastMinute    uint64        `ms:"connection_bandwidth_received_last_minute_total"`
+}
+
+// convert duration nanoseconds to seconds


This would break if called multiple times, see the discussion on decoding below.

stevenh · 2020-04-06T21:58:26Z

server_cmds.go

+}
+
+// convert duration nanoseconds to seconds
+func (obj *Server) Info() {


This would break if called multiple times, see the discussion on decoding.

mightyBroccoli marked this pull request as ready for review October 17, 2019 10:03

3339513

* change bool to int serverinfo does not provide any boolean info

stevenh suggested changes Oct 18, 2019

View reviewed changes

fix malaligned struct

1603b8c

* alignment fixup

mightyBroccoli requested a review from stevenh February 10, 2020 14:05

stevenh suggested changes Mar 27, 2020

View reviewed changes

mightyBroccoli added 5 commits April 3, 2020 12:15

gitignore

2b59f75

+ add proper gitignore file

ajust to the change requests

de416ee

* update mockserver command output to better represent the latest server version * changed any uptime variable to use time.duration * update server_cmds_test file to represent the changes done to server_cmds.go

naming inconsistency

6300cde

* fix naming inconsistancy

type changes

aef3073

* ajust port types to uint16

even more typing

a7f6d12

* ajust "total" values to use uint16 instead of int * ajust timestamps to use uint64 to give some headroom

stevenh approved these changes Apr 3, 2020

View reviewed changes

stevenh suggested changes Apr 3, 2020

View reviewed changes

mightyBroccoli and others added 5 commits April 3, 2020 21:00

Update .gitignore

45a609b

Co-Authored-By: Steven Hartland <[email protected]>

proper code comments

aba9174

* update code commentary to feature proper sentences

tpying changes

4e5e81d

* change bytes downloaded/uploaded values to uint64

change flags

6985b12

* change flags to use boolean reponses

final fixup

0ba8e34

* change ServerNickname to Nickname * change LogPermissions to boolean * fixed LogQuery to actually use virtualserver_log_query

mightyBroccoli requested a review from stevenh April 6, 2020 11:00

stevenh suggested changes Apr 6, 2020

View reviewed changes

mightyBroccoli added 2 commits April 6, 2020 16:35

correcting mistakes

8d25ba5

* partly reverse 0d3d6262590d0 serverrequestconnectioninfo was actually the correct call but the array I assign wasn't this fixes the array and correcty my rookie mistake

durations ns 2 s

bb0a675

+ add struct function to convert time.duration nanoseconds to seconds

stevenh suggested changes Apr 6, 2020

View reviewed changes

mightyBroccoli closed this by deleting the head repository Aug 11, 2024

query command overhaul #29

query command overhaul #29

Uh oh!

Conversation

mightyBroccoli commented Oct 17, 2019

Uh oh!

mightyBroccoli commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenh commented Oct 18, 2019

Uh oh!

stevenh left a comment

Choose a reason for hiding this comment

Uh oh!

mightyBroccoli commented Oct 18, 2019

Uh oh!

mightyBroccoli commented Feb 12, 2020

Uh oh!

mightyBroccoli commented Feb 26, 2020

Uh oh!

stevenh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenh left a comment

Choose a reason for hiding this comment

Uh oh!

mightyBroccoli commented Apr 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mightyBroccoli commented Oct 17, 2019 •

edited

Loading