remijouannet / graftorio2

(fork of graftorio) factorio mod for creating grafana dashboard
MIT License
77 stars 21 forks source link

New metrics like pollution, evolution and more #24

Closed Kariton closed 1 year ago

Kariton commented 1 year ago

changes as committed.

Have you worked with submodule before?

# git submodule update --init > download "linked" submodule

# git submodule update --remote > update submodule ("unlink")

this "unlink" is basically a git change. you can commit and push this "update submodule to commit hash X" and other will get THIS submodule version if they run --init (this does not add the files to you repository - just the referenced default 'init commit hash')

at the end everyone is able to change the submodule version on their own, but the init version is provided by this repository.

would you mind including this in your release routine? what do you think?

remijouannet commented 1 year ago

Hello,

thx for your PR, i would prefer not having to rely on git submodule for this, it's way too much complexity for a simple game mod, i would happily merge any dashboard update you have

also can you please split this into multiple PR

Kariton commented 1 year ago

Hey @remijouannet,

I understand your concern about submodule. After thinking about it, considering my own use case and nearly instantly getting a issue / request on my dashboards (prior to be a separate repository) I highly favor submodule and appreciate its convenience:

The discovered issue was related to the hardcoded prometheus UID which did not fit the existing deployed "stack". (which was no "real" problem and easy to resolve)

Hence the idea to provide a dedicated repository that can be utilized as submodule (or flat files in the alternative case). Submodule is how I add those dashboards to my "stack". More experienced (or learning) users are likely to add the dashboards to their already existing deployments in some way.

Integrating them by dismantling this repository is IMHO not feasible.

Not so experienced users should hopefully be able to follow the "download / unzip" or "two command" way.

Another benefit is that a separate repository will keep your commits and issues cleanisch.

Don't get me wrong. I am very pleased about the offer to "merge any dashboard update"! But the alternative solution is that I update "my" dashboard repository and copying the same files to this repository... I mean... that's what submodule is made for.

Maybe that could change your mind a bit.

anyway: Do you want completely new PRs or can I "downgrade" this one to include only new_metrics ?

remijouannet commented 1 year ago

i'm sorry, i don't think the number of change and issues generate over time by your dashboards can justify setting up submodule, if i'm wrong i will reconsidere my opinion.

you can still maintain your own dashboard and publish them on your personal repo or on https://grafana.com/grafana/dashboards/

yes you can downgrade this PR, please rebase it to new_metrics only

Kariton commented 1 year ago

well.... load active_mods and surfaces only on_load -> crashes on startup attempt to index global 'game' (a nil value) and the research change crashes too -> crashes on startup attempt to index global 'player' (a nil value)

prior to those changes it did work as intended.

Kariton commented 1 year ago

TBH I don't know why or what I can do about that.

remijouannet commented 1 year ago

https://lua-api.factorio.com/latest/LuaBootstrap.html#LuaBootstrap.on_load https://lua-api.factorio.com/latest/LuaBootstrap.html#LuaBootstrap.on_init move it to on_init

Kariton commented 1 year ago

Thanks for your feedback.

I tested the on_init successfully. and the variable thing is so obvious...

Kariton commented 1 year ago

load active_mods and surfaces only on_init, fix research_queue

This does not work this way.

on_init This is only called when a new save game is created or when a save file is loaded that previously didn't contain the mod.

My game now never exports this metrics for seed and mods.

A possible solution would be to revert this and wrap it in a condition. events.lua

local is_exported = {
    surfaces = false,
    mods = false,
}

function register_events(event)
    gauge_tick:set(game.tick)

    if not is_exported.surfaces then
        for _, surface in pairs(game.surfaces) do
            gauge_seed:set(surface.map_gen_settings.seed, {surface.name})
        end
        is_exported.surfaces = true
    end

    if not is_exported.mods then
        for name, version in pairs(game.active_mods) do
            gauge_mods:set(1, {name, version})
        end
        is_exported.mods = true
    end

    for _, player in pairs(game.players) do
        stats = {
            { player.force.item_production_statistics, gauge_item_production_input, gauge_item_production_output },
[...]

But it seems like it exports them anyway each run... What do you think?

EDIT: Formatting

Kariton commented 1 year ago

Does the export happen because it persistently saved through gauge_*:set?

Kariton commented 1 year ago

Hey @remijouannet,

Yesterday I actually used the graftorio fork from TheVirtualCrew. This is the source of my snippets to add more metrics.

I then quickly discovered that their fork does contain a lot more metrics and lables then expected. and yet not so UPS intense to run. My base test base is quite big and graftorio2, it its current state, does hit my UPS on every export. The explanation for this is quiet easy: gathering all facts at once.

The other version does handle that way better and splits the gathering in stages.

I then ported the code to my fork while reviewing your change history to reimplement fixes and features. https://github.com/Kariton/graftorio2/tree/TheVirtualCrew_port

would you mind looking into that branch, so we can drop this PR in favor of an new one? I really appreciate your work and invested time. But I think this could bring the mod to a totally new level.

remijouannet commented 1 year ago

this PR is not the place to discuss this, please focus on what this PR was intended to : adding pollution, mods, seed, research

remijouannet commented 1 year ago

on_init is execute everytime you're loading your save, isn't it enough ?

Kariton commented 1 year ago

control-stage on_init is only triggered once - when the mod is new. if you later on add new mods they are not exported.

remijouannet commented 1 year ago

i've merged the PR because i don't want it to stall, i'll looked into the issue for "game_mods"

Kariton commented 1 year ago

Hey, thank you for that. Sorry for the stalled issue.

Sadly my PC blew up. But a new one is on its way.

I still want to contribute more metrics and update the dashboards accordingly.

In the meantime you may want to rename the dashboards because windows.... replace all non alnum chars. For example: '11DefaultElectricity.json' This is intended to be a commit I would make in the future.

Greetings, Kariton


09.03.2023 22:44:52 Remi Jouannet @.***>:

i've merged the PR because i don't want it to stall, i'll looked into the issue for "game_mods"

— Reply to this email directly, view it on GitHub[https://github.com/remijouannet/graftorio2/pull/24#issuecomment-1462867604], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AQCYKFEJMIDYID2I2OX52VDW3JFNFANCNFSM6AAAAAATKDXXF4]. You are receiving this because you authored the thread.[Verfolgungsbild][https://github.com/notifications/beacon/AQCYKFHHGKLPOXMS4FZWIFDW3JFNFA5CNFSM6AAAAAATKDXXF6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSXGGLJI.gif]