Skip to content

rework bstats to use metrics base#6882

Closed
MiniDigger wants to merge 1 commit into
masterfrom
rework-bstats
Closed

rework bstats to use metrics base#6882
MiniDigger wants to merge 1 commit into
masterfrom
rework-bstats

Conversation

@MiniDigger
Copy link
Copy Markdown
Member

@MiniDigger MiniDigger commented Nov 11, 2021

  • removes a ton of our custom code and replaces it with the metrics base lib
  • if we create the bstats config, we now send a bit of info to inform users that metrics are opt-out
  • I think this fixes the "outdated" versions chart on the bstats global page, as we never did send bukkitVersion and bukkitName

@MiniDigger MiniDigger requested review from a team as code owners November 11, 2021 07:29
Comment thread patches/server/0008-Paper-Metrics.patch Outdated
@MiniDigger MiniDigger force-pushed the rework-bstats branch 2 times, most recently from 93f8fc5 to 1940342 Compare November 11, 2021 07:44
@MiniDigger
Copy link
Copy Markdown
Member Author

{
    "playerAmount": 0,
    "onlineMode": 1,
    "bukkitVersion": "git-Paper-\"93f8fc5\" (MC: 1.17.1)",
    "bukkitName": "Paper",
    "javaVersion": "16",
    "osName": "Windows 10",
    "osArch": "amd64",
    "osVersion": "10.0",
    "coreCount": 24,
    "service": {
        "id": 580,
        "customCharts": [
            {
                "chartId": "minecraft_version",
                "data": {
                    "value": "1.17.1"
                }
            },
            {
                "chartId": "legacy_plugins",
                "data": {
                    "values": {
                        "0 ?": {
                            "0": 1
                        }
                    }
                }
            },
            {
                "chartId": "online_mode",
                "data": {
                    "value": "online"
                }
            },
            {
                "chartId": "paper_version",
                "data": {
                    "value": "git-Paper-\"93f8fc5\""
                }
            },
            {
                "chartId": "java_version",
                "data": {
                    "values": {
                        "Java 16": {
                            "16": 1
                        }
                    }
                }
            }
        ]
    },
    "serverUUID": "c6aa11bc-2f20-46de-8808-4cb98dce6518",
    "metricsVersion": "2.2.1"
}

@NoahvdAa
Copy link
Copy Markdown
Member

                "chartId": "legacy_plugins",
                "data": {
                    "values": {
                        "0 ?": {
                            "0": 1
                        }
                    }
                }

is 😎 being properly encoded here, since it's showing up as a question mark?

@MiniDigger
Copy link
Copy Markdown
Member Author

MiniDigger commented Nov 11, 2021

mmh, idk, I wanna blame the logging (edit: yeah, internally its fine)

also gotta relocate stuff, idk why I did to choose to ignore the relocate check, lmao

@MiniDigger
Copy link
Copy Markdown
Member Author

ok, should be good to go now, ideally @Bastian can take a look, that would be very much appreciated, especially if this is right, because we didn't do that before https://github.com/PaperMC/Paper/blob/rework-bstats/patches/server/0008-Paper-Metrics.patch#L190-L193

@Bastian
Copy link
Copy Markdown
Contributor

Bastian commented Nov 12, 2021

Sending this extra data has zero effect and are just ignored by bStats.
The only fields parsed by bStats are "coreCount", "osArch", "osVersion", and "os" . Everything else is a default chart.

Comment thread patches/server/0008-Paper-Metrics.patch Outdated
+ public Metrics() {
+ File bStatsFolder = new File((File) MinecraftServer.getServer().options.valueOf("plugins"), "bStats");
+ File configFile = new File(bStatsFolder, "config.yml");
+ MetricsConfig config;
Copy link
Copy Markdown
Contributor

@Bastian Bastian Nov 12, 2021

Choose a reason for hiding this comment

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

Unfortunately, you cannot use the MetricsConfig. You still have to create and parse the config file yourself.
The MetricsConfig is not a YAML file but uses its own format that looks like this and is not compatible with the existing config files:

# bStats (https://bStats.org) collects some basic information for plugin authors, like
# how many people use their plugin and their total player count. It's recommended to keep
# bStats enabled, but if you're not comfortable with this, you can turn this setting off.
# There is no performance penalty associated with having metrics enabled, and data sent to
# bStats is fully anonymous.");
enabled=true
server-uuid=<UUID>
log-errors=false
log-sent-data=false
log-response-status-text=false

(see source: MetricsConfig.java)

This config is only intended for new platforms and cannot be used by Paper for legacy reasons.
For the config, you should use the Bukkit Metrics class as a reference: Metrics.java#L39-L64

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ahh, seeing that bukkit didn't use it confused me, that makes sense.
alternatively we could migrate existing files? ah no, cause other plugins, meh. ok, will fix later

@Strahilchu
Copy link
Copy Markdown

Wait you guys keep bstats and metrics enabled?

@Bastian
Copy link
Copy Markdown
Contributor

Bastian commented Nov 12, 2021

Wait you guys keep bstats and metrics enabled?

bStats is opt-out on any platform except Sponge.

@MiniDigger
Copy link
Copy Markdown
Member Author

Wait you guys keep bstats and metrics enabled?

bstats always has been and always will be enabled by default on paper, but you can easily disable it before it first sends data. This PR will even make it send a headsup message on first start

metrics on bstats are extremly valuable to us, it's how we decide which java/mc versions to support for example, or see how quickly new (paper/mc/java) versions are addapted. It's also valuable to get an estimate on market share.

They are also fully anonymous, don't disclose any personal data, and will not create any lag (obviously, lol), so there is no reason to disable them, ever, altho we do respect that choice if you wanna make that (altho we wouldn't understand it).

(side-note, if you are concerned about this, go audit all your plugins, the plugins that don't use bstats for usage stats often collect intrusive information like server name, server IP, plugin names etc, those we would never allow happening to our users)

Copy link
Copy Markdown
Contributor

@Bastian Bastian left a comment

Choose a reason for hiding this comment

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

Looks good to me.

If you didn't already, I would recommend you to build it once and enable the logSentData and logResponseStatusText just to be sure that the correct data is sent and the bStats backend accepts it.

@MiniDigger
Copy link
Copy Markdown
Member Author

Looks good to me.

If you didn't already, I would recommend you to build it once and enable the logSentData and logResponseStatusText just to be sure that the correct data is sent and the bStats backend accepts it.

thanks for taking a look!
did that already :)

@MiniDigger MiniDigger added status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. and removed status: rebase required labels Dec 30, 2021
@MiniDigger
Copy link
Copy Markdown
Member Author

somebody with more knowledge about bundler and stuff needs to figure out relocation and shadow here please

relocate("org.bukkit.craftbukkit" to "org.bukkit.craftbukkit.v$packageVersion") {
exclude("org.bukkit.craftbukkit.Main*")
}
+ relocate("org.bstats:bstats-base" to "org.bstats") // Paper
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we don't relocate libs anymore right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Warriorrrr Warriorrrr moved this from Awaiting final testing to Waiting For Author in Paper PR Queue Mar 5, 2025
@kennytv kennytv added the pre-softspoon: never rebased Pre-hardfork pull requests that were not re-opened with the new main branch label Mar 23, 2025
@kennytv kennytv deleted the branch master March 23, 2025 19:15
@kennytv kennytv closed this Mar 23, 2025
@kennytv kennytv deleted the rework-bstats branch March 23, 2025 19:17
@kennytv kennytv restored the rework-bstats branch March 23, 2025 19:17
@kennytv kennytv deleted the rework-bstats branch March 23, 2025 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pre-softspoon: never rebased Pre-hardfork pull requests that were not re-opened with the new main branch pre-softspoon status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. status: rebase required

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants