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

Convert SpigotMC module to use regexes for parsing. #5507

Merged
merged 9 commits into from Mar 6, 2019

Conversation

@Ferroin
Copy link
Collaborator

Ferroin commented Feb 25, 2019

Summary

This converts the SpigotMC python module to use regular expressions for parsing, which makes the parsing a bit more robust at the cost of some performance.

Additionally, this relaxes the parsing logic surrounding the handling of the user counts to just grab the first integer after the timestamp and treat it as the count of users.

Component Name

python.d.plugin / spigotmc.chart.py

Additional Information

Fixes: #4131


I've verified the regexes against sample console output I had saved for testing though, as well as the console outputs posted in #4131, so while I haven't directly tested the code myself (I don't have a working SpigotMC installation that I can test against right now, and I unfortunately don't have the time to get one working right now either) it should work, but it would be good to have someone who has a working SpigotMC installation test this.

This makes the parsing a bit more robust at the cost of some
performance.

Additionally, this relaxes the parsing logic surrounding the handling of
the user counts to just grab the first integer after the timestamp and
treat it as the count of users.

Fixes: #4131
@Ferroin Ferroin requested a review from ilyam8 Feb 25, 2019
@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 25, 2019

@Ferroin

([0-9]{1,2}.[0-9]{1,2})

are you sure about {1,2} ?

@Ferroin

This comment has been minimized.

Copy link
Collaborator Author

Ferroin commented Feb 25, 2019

For the integer part, yes, because the first number isn't guaranteed to be zero padded, but can't realistically be more than two digits.

For the decimal part, not so much, but I don't have any better suggestions (unless of course using a + is significantly faster, in which case it should probably be switched to that).

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 25, 2019

Idea

re.findall(
	r'([0-9]{1,2}\.[0-9]+)',
	'§6TPS from last 1m, 5m, 15m: §a19.99, §a19.99, §a19.99\n'
)
@Ferroin

This comment has been minimized.

Copy link
Collaborator Author

Ferroin commented Feb 25, 2019

@ilyam8 The problem is that I'm not 100% certain that won't match other things in the string too (the garbage bytes appear to be just that, consistent garbage, so i can't be sure they don't change). By just matching the whole line we also get a much nicer indication if the format ever changes (instead of randomly wrong values, it will refuse to parse).

@netdatabot

This comment has been minimized.

Copy link
Member

netdatabot commented Feb 26, 2019

This pull request introduces 1 alert when merging 896cfe7 into 81bf7a7 - view on LGTM.com

new alerts:

  • 1 for Syntax error

Comment posted by LGTM.com

Ferroin added 2 commits Feb 26, 2019
@ilyam8
ilyam8 approved these changes Feb 26, 2019
@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 26, 2019

@alsoGAMER if you have time please test it, you are the only spigotmc user in the world we know

@Ferroin

This comment has been minimized.

Copy link
Collaborator Author

Ferroin commented Feb 26, 2019

I'm still hoping to get some feedback one way or another from @alsoGAMER or someone else who is currently using the module on this.

Assuming it's OK with you @ilyam8, I plan to wait until next Monday, and if we still haven't heard anything by then, just merge it.

@netdata netdata deleted a comment Feb 26, 2019
@Ferroin Ferroin merged commit e7158e7 into netdata:master Mar 6, 2019
12 checks passed
12 checks passed
Header rules - netdata No header rules processed
Details
Pages changed - netdata 2 new files uploaded
Details
Redirect rules - netdata No redirect rules processed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
Mixed content - netdata No mixed content detected
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
netlify/netdata/deploy-preview Deploy preview ready!
Details
@Ferroin Ferroin deleted the Ferroin:spigotmc-fixes branch Mar 6, 2019
@elfhack

This comment has been minimized.

Copy link

elfhack commented Mar 9, 2019

Just a headsup, I dropped this version into a running 1.12.0 in the official docker image, and it errors out with a

2019-03-10 00:42:15: python.d ERROR: plugin: main: module load source: 'spigotmc' => [FAILED]
2019-03-10 00:42:15: python.d ERROR: plugin: main: load source error : 'module' object has no attribute 'A'

This is because the docker image uses py2, which has no re.A. I've removed it, and unfortunately it still errors out with "Unable to process"

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 10, 2019

@elfhack

i see

self.error('Unable to fetch TPS values.')

and

self.error('Unable to process user counts.')

I've removed it, and unfortunately it still errors out with "Unable to process"

TPS values or user counter?


plese add

self.info('RAW TPS :', raw)

right after

raw = self.console.command('tps')

self.info('RAW LIST :', raw)

right after

raw = self.console.command('list')

and

self.info('RAW ONLINE :', raw)

after

raw = self.console.command('online')

and run spigotmc module in debug mode, lets see what's the problem

https://docs.netdata.cloud/collectors/python.d.plugin/#how-to-debug-a-python-module

@elfhack

This comment has been minimized.

Copy link

elfhack commented Mar 10, 2019

@ilyam8
Did as you asked, i think the problem is somewhere deeper.

2019-03-10 14:49:09: python.d INFO: plugin: main: Using python 2
2019-03-10 14:49:09: python.d DEBUG: plugin: main: loading '/etc/netdata/python.d.conf'
2019-03-10 14:49:09: python.d ERROR: plugin: main: cannot load '/etc/netdata/python.d.conf': [Errno 2] No such file or directory: '/etc/netdata/python.d.conf'. Will try stock version.
2019-03-10 14:49:09: python.d DEBUG: plugin: main: loading '/usr/lib/netdata/conf.d/python.d.conf'
2019-03-10 14:49:09: python.d DEBUG: plugin: main: module load source: 'spigotmc' => [OK]
2019-03-10 14:49:09: python.d DEBUG: plugin: main: loading '/etc/netdata/python.d/spigotmc.conf'
2019-03-10 14:49:09: python.d DEBUG: plugin: main: job initialization: 'spigotmc spigot' => ['OK']
2019-03-10 14:49:09: python.d DEBUG: plugin: main: module status: 'spigotmc' => [OK] (jobs: 1)
2019-03-10 14:49:09: python.d INFO: spigotmc: spigot: check() => [OK]
CHART netdata.runtime_spigotmc_spigot '' 'Execution time for spigotmc_spigot' 'ms' 'python.d' netdata.pythond_runtime line 145000 5
DIMENSION run_time 'run time' absolute 1 1

2019-03-10 14:49:09: python.d DEBUG: spigotmc: spigot: create() => [OK] (charts: 2)
2019-03-10 14:49:09: python.d DEBUG: spigotmc: spigot: started, update frequency: 5
2019-03-10 14:49:13: python.d INFO: spigotmc: spigot: RAW LIST : 
2019-03-10 14:49:13: python.d ERROR: spigotmc: spigot: Unable to process TPS values.
2019-03-10 14:49:13: python.d INFO: spigotmc: spigot: RAW List: 
2019-03-10 14:49:13: python.d INFO: spigotmc: spigot: RAW Online: 
2019-03-10 14:49:13: python.d ERROR: spigotmc: spigot: Unable to process user counts.
2019-03-10 14:49:13: python.d DEBUG: spigotmc: spigot: get_data() returned no data
2019-03-10 14:49:13: python.d DEBUG: spigotmc: spigot: update => [FAILED] (elapsed time: -, failed retries in a row: 1)

However I can see it connecting to rcon (do not mind the hour difference, alpine is missing localtime)

[13:49:09 INFO]: Rcon connection from: /172.18.0.2
@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 10, 2019

well, i see

all these commands

raw = self.console.command('tps')

raw = self.console.command('list')

raw = self.console.command('online')

return None

@elfhack

This comment has been minimized.

Copy link

elfhack commented Mar 10, 2019

Yeah, but talking to rcon gives back what you'd expect

➜ docker exec spigot rcon-cli list
There are 0 of a max 20 players online: 

➜ docker exec spigot rcon-cli tps 
§6TPS from last 1m, 5m, 15m: §a20.0, §a19.99, §a19.99
@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 10, 2019

@elfhack

Take a look

I installed spigotmc

docker run -d -p 25575:25575 -e EULA=TRUE -e RCON_PASSWORD=testing --name mc itzg/minecraft-server

that is python script

import sys
import mcrcon

m = mcrcon.MCRcon()
m.connect('127.0.0.1', 25575, sys.argv[1])
print(m.command('list'))

result

[lgz :~]python spigot.py wrong

[lgz :~]python spigot.py testing
There are 0 of a max 20 players online: 
[lgz :~]

There is no authentication error or something, just no values

@elfhack

This comment has been minimized.

Copy link

elfhack commented Mar 10, 2019

@ilyam8

Hmm, I use the exact same container, but I don't declare any RCON_PASSWORD. I'll try defining one in a couple of minutes

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 10, 2019

but I don't declare any RCON_PASSWORD

this is the problem, it seems default is not ''

@elfhack

This comment has been minimized.

Copy link

elfhack commented Mar 10, 2019

Yeah, just grepped through my server.properties. That was dumb

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 10, 2019

ok, py2 compatibility issue fixed in #5593, will have it merged in a minute

@elfhack

This comment has been minimized.

Copy link

elfhack commented Mar 10, 2019

Yep, once i deleted the debug lines (python complained about converting some characters into ascii), it started working perfectly. Someone should leave some warning about this password thing :/

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 10, 2019

@elfhack
could you try to install #5593 and check if it works?

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 10, 2019

Someone should leave some warning about this password thing :/

i am not sure, it may be specific to itzg/minecraft-server, haven't tested other. Anyway i added some log messages about it

self.error("'{0}' command returned no value, make sure you set correct password".format(COMMAND_TPS))

@elfhack

This comment has been minimized.

Copy link

elfhack commented Mar 10, 2019

@ilyam8

Nope, can't use this version

2019-03-10 22:06:26: python.d ERROR: spigotmc: spigotmc: check() unhandled exception: 'ascii' codec can't encode character u'\xa7' in position 0: ordinal not in range(128)
2019-03-10 22:06:26: python.d ERROR: spigotmc: spigotmc: Traceback (most recent call last):
  File "/usr/libexec/netdata/plugins.d/python.d.plugin", line 319, in check_job
    check_ok = bool(job.check())
  File "/usr/libexec/netdata/python.d/spigotmc.chart.py", line 82, in check
    return self._get_data()
  File "/usr/libexec/netdata/python.d/spigotmc.chart.py", line 119, in _get_data
    self.debug("'{0}' command output : {1}".format(COMMAND_TPS, raw))
UnicodeEncodeError: 'ascii' codec can't encode character u'\xa7' in position 0: ordinal not in range(128)
@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 10, 2019

fixed in #5598

ilyam8 added a commit that referenced this pull request Mar 10, 2019
<!--
Describe the change in summary section, including rationale and degin decisions.
Include "Fixes #nnn" if you are fixing an existing issue.

In "Component Name" section write which component is changed in this PR. This
will help us review your PR quicker.

If you have more information you want to add, write them in "Additional
Information" section. This is usually used to help others understand your
motivation behind this change. A step-by-step reproduction of the problem is
helpful if there is no related issue.
-->

##### Summary

Fix [UnicodeDecodeError](#5507 (comment))

##### Component Name
[`collectors/python.d.plugin/spigotmc`](https://github.com/netdata/netdata/tree/master/collectors/python.d.plugin/spigotmc)
##### Additional Information
@elfhack

This comment has been minimized.

Copy link

elfhack commented Mar 10, 2019

@ilyam8
Yep, this one works from the get go. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.