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

Add per-player stats #34

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Zincfox
Copy link
Contributor

@Zincfox Zincfox commented Jul 19, 2024

Adds the following metrics (can be disabled in settings):

  • factorio_player_connected[name]: 1 if the player is currently connected
  • factorio_player_last_online[name]: Last tick the player was connected
  • factorio_player_time_online[name]: Time (in seconds) the player spent online
  • factorio_player_position_x[name]: The x coordinate of the players (last) position
  • factorio_player_position_y[name]: The y coordinate of the players (last) position
  • factorio_player_position_surface[name, surface]: The surface of the players (last) position

Zincfox and others added 2 commits July 19, 2024 02:23
Adds the following metrics (can be disabled in settings):
- factorio_player_connected[name]: 1 if the player is currently connected
- factorio_player_last_online[name]: Last tick the player was connected
- factorio_player_time_online[name]: Time (in seconds) the player spent online
- factorio_player_position_x[name]: The x coordinate of the players (last) position
- factorio_player_position_y[name]: The y coordinate of the players (last) position
- factorio_player_position_surface[name, surface]: The surface of the players (last) position
@Zincfox
Copy link
Contributor Author

Zincfox commented Jul 19, 2024

Given that player names are in theory a high cardinality label, these new metrics should maybe be disabled by default, like the train metrics mentioned in #32 (although this case should be way less severe).
It seems a bit weird to me to have a disable by default be set to true (ideally would be enable set to false), but I believe renaming the setting would probably cause a conflict with existing setups? Obviously not a problem for this PR, but for the train one, the style of which should be matched by this one's new setting.

gauge_player_last_online =
prometheus.gauge("factorio_player_last_online", "last tick the player was online", { "name" })
gauge_player_time_online =
prometheus.gauge("factorio_player_time_online", "total seconds player spent online", { "name" })
Copy link
Owner

Choose a reason for hiding this comment

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

i believe it should be a counter and not a gauge, the value can only go up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I was just matching the other calls - as far as I could tell (although I did not look at every file), all other currently added metrics are either gauges or histograms (even gauge_tick, of which these two are kind of 'subsets', so to speak).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also on that note, gauge_player_time_online and gauge_tick should be actively reset to 0 on start if they are turned into counters, no? Otherwise if the server crashes without saving, but still exported its metrics at least once since the last save, the observed value could appear to decrease on the real-world timescale, even if it is monotonically increasing in relation to game-ticks.

Another interpretation (aside from just "this case is irrelevant") could be that rate(factorio_tick) should not be used, because the value can, exactly as stated by its gauge nature, go down?

I will wait for further confirmation on this before changing the metrics to counters. If you want I can also go over the existing metrics and change their type where appropriate, although I dont know how well prometheus handles existing metrics changing their type.

@Zincfox
Copy link
Contributor Author

Zincfox commented Aug 5, 2024

Please dont merge this yet. Now that #37 is merged I can also include documentation for the metrics added in this PR to the Metrics.md file.

Update: Done

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