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

stats: worker count and list #81

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Conversation

gavinbarnard
Copy link
Contributor

@gavinbarnard gavinbarnard commented Apr 4, 2021

Add support for the xmrig --rig-id option, add worker_count as a statistic in /stats, and a list of up to as many connected rigs that will fit in a 1mb buffer

I know this is probably terrible, please be gentle, my c skills are rusty.

Copy link
Owner

@jtgrassie jtgrassie left a comment

Choose a reason for hiding this comment

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

My main concern with this is that it only caters to small scale miners (<32 workers), but there are also some inefficiencies with how this is implemented and a load of indentation issues.

src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/webui.c Show resolved Hide resolved
src/pool.h Outdated Show resolved Hide resolved
src/pool.c Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
@jtgrassie
Copy link
Owner

I know this is probably terrible, please be gentle, my c skills are rusty.

It's not terrible, so thank you for submitting. I tried to capture all my comments in the review above.

The scale point is tricky because some miners have 100s/1000s of workers, thus 32 just isn't enough. As this just returns a list of online worker IDs, it may make more sense to split it off to a different endpoint (e.g. /workers) and then let it be a 1 MB buffer, which can then return ~25k workers.

@gavinbarnard
Copy link
Contributor Author

Thank you for your time to review. I will work to clean up all the bad indentations, and start using a proper IDE. This was all done with vim on my stage system.

I was only supporting listing the first 32 found rig_id from the client. As well yes I am running a fairly small pool, but I guess it is wise to think of the larger scale.

Copy link
Owner

@jtgrassie jtgrassie left a comment

Choose a reason for hiding this comment

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

Per above comments, scale is still an issue. The worker count function can take start/end pointers to 1 MB buffer.

src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.h Outdated Show resolved Hide resolved
src/pool.h Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/pool.h Outdated Show resolved Hide resolved
src/webui.c Outdated Show resolved Hide resolved
src/webui.c Outdated Show resolved Hide resolved
src/webui.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/webui.c Outdated Show resolved Hide resolved
src/pool.c Outdated Show resolved Hide resolved
src/webui.c Outdated Show resolved Hide resolved
@jtgrassie
Copy link
Owner

jtgrassie commented Apr 5, 2021

Please squash commits and rebase after the last change please. Thanks. Oh, and rename, stats: worker count and list.

@gavinbarnard gavinbarnard changed the title extra logging, rig id, and worker_count stats: worker count and list Apr 6, 2021
@jtgrassie
Copy link
Owner

Super. Thank you. I'll do some tests at the weekend and merge then.

@jtgrassie
Copy link
Owner

jtgrassie commented Apr 9, 2021

Please remove the new commit (5ff5735). If you want to add more functionality after I've merged this PR, you can open a new PR. Thanks.

jtgrassie added a commit that referenced this pull request Apr 9, 2021
 - buffer size (for systems with smaller thread stack)
 - terminating NULL in rig-id copy
 - rename functions/variables
 - reorder functions
 - minor formatting issues
@jtgrassie jtgrassie merged commit 127b349 into jtgrassie:master Apr 9, 2021
@gavinbarnard gavinbarnard deleted the rig_worker branch April 10, 2021 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants