SpongePowered / SpongeAPI

A Minecraft plugin API
http://www.spongepowered.org/
MIT License
1.14k stars 342 forks source link

Rework of the Metrics API (Missing undecided config value) #1966

Open BrainStone opened 5 years ago

BrainStone commented 5 years ago

As per the discussion on the Discord, I'm writing this down as an issue/feature request.

Per older and newer discussion it was initially agreed upon that Sponge provided an API for plugins to check if they may send metrics. And that they are supposed to ask for consent. The API was added in a very basic sense. The issue being the plugins only knowing if metrics are enabled or not. Meaning that even if an admin purposely disabled them, they would get asked every time, if a plugin ever decided to do that notification.

That's the core issue. Whie discussing it several suggestions came up to streamline the process and provide a nice and consitent UI to the user/admin.

I propose the following:

This is to allow people to give general consent or dissent, for the ones that really don't want to be bothered by individual prompts and also to give those who want it, full controll out of the box.
This natually needs commands to allow changing all these settings directly, so they can be used in clickable links/texts.

Now this shouldn't be too hard to implement.

In addition I had a few other suggestions to improve usability:

Now I would kindly ask that if that's the way it'll be solved, that the message is worded something like

Would you like to allow plugins to collect anonymized metrics? This helps the developers improve their plugins.  
Yes - No - Please ask me individually

Essentially asking politely to enable it while pointing out it helps the devs make the plugin better.

Faithcaio commented 5 years ago

Not sure if this is still the case but I noticed Sponge was adding values to the config for every plugin loaded. We don't need to save undecided for plugins that dont even have metrics.

BrainStone commented 5 years ago

That's a good point. And naturally these prompts should only appear when plugins check if they may send metrics and individual prompts only for thos which check for it. Makes it also easier seeing which plugins do send metrics.

ImMorpheus commented 5 years ago

https://github.com/SpongePowered/SpongeAPI/pull/2018

ST-DDT commented 5 years ago

The conditional requests sound good to me, however we should consider the timing when we will send them. If we send them "every time" a plugin checks its permission then we might annoy the admins. If we check it only during the first time no admin might be only at all. Maybe prompting this after joining the server might be an idea. Only if the first request is answered will the next one be shown. How should we handle cases where admins only "activate" their permissions with a special command?

BrainStone commented 5 years ago

I think the best way to time it would be to check a permission when a player joins and then display the prompts that are there to be shown. One after the other. Ideally changing a plugins setting would send an event the plugin or stats library can listen to.

dualspiral commented 5 years ago

For what it's worth, this was discussed a while ago and the guidelines we're going to strive for are https://drive.google.com/file/d/16kbuFVtuKasrKLTNb6gIOc-BOEyWBnFa/view (it does say we're striving for API 8, but if it hits 7.2 too, hey, so much the better).

Note that "conditional requests" are actually against the guidelines and should be handled by Sponge instead. It does call into question what the "undefined" state is for, but I'm not particularly against it being there either.

jamierocks commented 4 years ago

Move all the metrics stuff into it's own config

👍 would love to see a metrics.conf in API 8.

dualspiral commented 4 years ago

The associated PR was merged, but there seems to be more here than just what that PR proposed. I'm going to slap at 1.14 label on it now though, particularly with that last comment.

ImMorpheus commented 3 years ago

metrics.conf was added by https://github.com/SpongePowered/Sponge/commit/ad94e11623b4d9e33769151e58be39c55acf323f

dualspiral commented 3 years ago

This shouldn't have been closed - as well as the config not being an API element, there are other aspects of this that should be discussed.

This issue is not about the config file, it's about the wider API and prompt issues from a plugin standpoint, from above.

  • Actually implement the prompts in Sponge itself. This has quite a few advantages:

    • You make it a lot easier for plugin and stats devs to work with the restrictions. Like that basically the same stuff doesn't need to be implemented a billion times over
    • It would make sure the messages and prompts are standardized and consitent. Having every plugin have their own style and timing can get super annoying
    • If there ever will be a rivaling metrics system to bStats (or some dev makes their own custom) the user doesn't get two messages for the global setting
    • It might even be possible to prevent plugins executing the commands on behalf of users this way, as it would be all Sponge internal.

This may be better off on the Sponge repo than the API one, but this issue is not resolved yet.

ImMorpheus commented 3 years ago

I missed the prompt part, good catch.