Add base bstats to "getting started" guide
Andre601 opened this issue · 10 comments
There should be a mention of bstats-base which can be used for creating the various Pie Charts.
I myself found it only after looking at this repo and thinking, if there's a base download since this repo has a base folder.
It would help a lot of devs who have a plugin supporting several platforms, as they could have a central method used to create and return the different charts to then implement into the more platform-specific Metrics.
While at the topic wouldn't it be somewhat of an idea to make the Metrics class also more platform-agnostic?
I can imagine a lot of the statistics being pretty much the same across all platforms (OS, Plugin version, etc.) and a simple applyMetrics method on the platform-specific bstats could then apply the right info to that Metric.
Example:
// Main/generic module of the plugin.
public class Core{
// All the stuff done by the core
public Metrics loadMetrics(int pluginId){
Metrics metrics = new Metrics(pluginId); // create metrics for this plugin
metrics.addCustomChart(new BasicChart("is_awesome", () -> "yes"));
return metrics;
}
}
// Spigot Module
public class SpigotPlugin extends JavaPlugin{
// onEnable(), onDisable(), blablabla...
public void loadMetrics(){
Metrics metrics = core.loadmetrics(123456); // core is main class
BStats.applyMetrics(metrics, this); // Apply platform-specific metrics
}
}
// BungeeCord Module
public class BungeePlugin extends Plugin{
// onEnable(), onDisable(), blablabla...
public void loadMetrics(){
Metrics metrics = core.loadmetrics(234567); // core is main class
BStats.applyMetrics(metrics, this); // Apply platform-specific metrics
}
}The applyMetrics method would take the Metrics class and apply all platform-specific information to it.
That way would the platform-specific BStats only require necessary methods to apply those metrics to a generic metrics class to then send to BStats.
This could reduce duplicated code for both the dev and BStats, because I see no reason as to why Metrics has to be platform-specific when it could be platform-agnostic and be a central part rather than duplicated across all platforms.
They base module is coming with the other Metrics classes as a transitive dependency – you should not include it yourself.
This could reduce duplicated code for both the dev and BStats
The single Metrics classes are already only containing platform specific code and nothing else. Everything platform agnostic (e.g. data sending, the charts, etc.) is already part of the base module. Take a look at the Velocity Metrics class: https://github.com/Bastian/bStats-Metrics/blob/master/velocity/src/main/java/org/bstats/velocity/Metrics.java
Reducing duplicate code was the big goal of the 2.x rewrite.
Your example would already work with only very little adjustments:
public class Core {
public void applyCustomCharts(Consumer<CustomChart> consumer) {
consumer.accept(new PieChart("foo", () -> "bar");
consumer.accept(new PieChart("bar", () -> "foo");
}
}
public class SpigotPlugin extends JavaPlugin{
// onEnable(), onDisable(), blablabla...
public void loadMetrics() {
// The Metrics class imported from org.bstats.bukkit.Metrics
Metrics metrics = new Metrics(123456);
core.applyCustomCharts(metrics::addCustomChart);
}
}
public class BungeePlugin extends JavaPlugin {
// onEnable(), onDisable(), blablabla...
public void loadMetrics( ){
// The Metrics class imported from org.bstats.bungeecord.Metrics
Metrics metrics = new Metrics(123456);
core.applyCustomCharts(metrics::addCustomChart);
}
}Disclaimer: I haven't actually tested it, but I see no reason why it shouldn't work as the custom charts all live in the base module in the org.bstats.charts package.
Your example assumes that everything is in the same core module, which may not always be the case.
My suggestion assumes that the dev(s) in question don't have a general/core module for all platforms but have it split up into different modules for reasons such as removing pointless imports.
Your example may work on a single-module setup, but would fail on a multi-module one, as the core module, which both platform ones depend on, would either need to have all platform-specific bstats-downloads or some other wanky method to implement bstats.
Having the core use all platform-specific downloads would not be a good solution as you would essentially have unwanted bloat in your plugin modules (Imports of bstats for spigot on bungeecord and vice-versa).
This wouldn't be optimal, so a more platform-agnostic approach should be desired.
Updated my comment a bit
Hmm, you are right, that's suboptimal.
Having the core use all platform-specific downloads would not be a good solution as you would essentially have unwanted bloat in your plugin modules (Imports of bstats for spigot on bungeecord and vice-versa).
The platform-specific Metrics classes are very small (100-200 lines including comments and empty lines) and would probably only increase the Jar size by less than 1kB. Additionally, with the minimizeJar option of the Maven Shade Plugin, it would purge all unused classes from your final jar and thus not increase the jar size at all.
That being said, I see why one might not want to clutter their core module with platform-specific Metrics classes or not use the minimizeJar option of the Maven Shade Plugin.
In your case, it's probably fine to include the base module in your core module. However, I don't want to actively promote the use of the base module as it was never intended to be included on its own.
Yeah. I currently struggle with this tho as I get a NPE on startup when I try to load the metrics... And I'm not sure if I could just relocate the bstats dependency on core without problems, or if I would need to do another relocation later on...
Any ideas?
This is the error I btw get when loading/initializing metrics (Tested on Velocity):
[10:41:03 ERROR]: Some errors occurred whilst posting event ProxyInitializeEvent.
[10:41:03 ERROR]: #1:
java.lang.NullPointerException: null
at com.andre601.oneversionremake.velocity.VelocityCore.loadMetrics(VelocityCore.java:87) ~[?:?]
at com.andre601.oneversionremake.core.OneVersionRemake.start(OneVersionRemake.java:138) ~[?:?]
at com.andre601.oneversionremake.core.OneVersionRemake.<init>(OneVersionRemake.java:48) ~[?:?]
at com.andre601.oneversionremake.velocity.VelocityCore.initialize(VelocityCore.java:63) ~[?:?]
at net.kyori.event.asm.generated.d0ce451b9d.VelocityCore-initialize-ProxyInitializeEvent-1.invoke(Unknown Source) ~[?:?]
at net.kyori.event.method.SimpleMethodSubscriptionAdapter$MethodEventSubscriber.invoke(SimpleMethodSubscriptionAdapter.java:148) ~[velocity.jar:1.1.4]
at net.kyori.event.SimpleEventBus.post(SimpleEventBus.java:107) ~[velocity.jar:1.1.4]
at com.velocitypowered.proxy.plugin.VelocityEventManager.fireEvent(VelocityEventManager.java:137) ~[velocity.jar:1.1.4]
at com.velocitypowered.proxy.plugin.VelocityEventManager.lambda$fire$1(VelocityEventManager.java:119) ~[velocity.jar:1.1.4]
at java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1700) ~[?:?]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) ~[?:?]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) ~[?:?]
at java.lang.Thread.run(Thread.java:834) [?:?]
Found a workaround. I just create the HashMap in the core module, so that I can just have metrics.addCustomChart(newDrilldownPie("allowed_protocols", () -> core.getPieMap());
@Andre601
There was a fundamental flaw in your code which resulted in this to begin with.
The exception you provided is a NullPointerException which means that the variable you were calling a method on pointed to null and was not set.
When we look at the stacktrace we see
com.andre601.oneversionremake.velocity.VelocityCore.loadMetrics(VelocityCore.java:87)
which points to this line:
https://github.com/Andre601/OneVersionRemake/blob/b555059f8349cdc424a5ecd6217b100c9bbfbffd/velocity/src/main/java/com/andre601/oneversionremake/velocity/VelocityCore.java#L87
metrics.addCustomChart(core.getPie());Now there are two possibilities for what could be null here.
Either metrics or core.
metrics is created using factory.make(this, 10341) and since that line did not throw a NPE, we can be sure that factory wasn't null and the Method Metrics.Factory#make(...) is provided by bStats and returns a call to the Metrics constructor.
So we should be able to rule that out for now too.
Now core could still be null. So when we look at when core is initialized, we find this:
https://github.com/Andre601/OneVersionRemake/blob/b555059f8349cdc424a5ecd6217b100c9bbfbffd/velocity/src/main/java/com/andre601/oneversionremake/velocity/VelocityCore.java#L63
@Subscribe
public void initialize(ProxyInitializeEvent event){
this.core = new OneVersionRemake(this);
}This may look fine at first but let's continue walking through the stacktrace where we find:
com.andre601.oneversionremake.core.OneVersionRemake.<init>(OneVersionRemake.java:48)
Aha, so the Metrics code is invoked when the constructor of OneVersionRemake is invoked.
And indeed, when we look at that constructor we find it leading to a call of start() which calls loadMetrics().
https://github.com/Andre601/OneVersionRemake/blob/b555059f8349cdc424a5ecd6217b100c9bbfbffd/core/src/main/java/com/andre601/oneversionremake/core/OneVersionRemake.java#L48
https://github.com/Andre601/OneVersionRemake/blob/b555059f8349cdc424a5ecd6217b100c9bbfbffd/core/src/main/java/com/andre601/oneversionremake/core/OneVersionRemake.java#L138
One thing you need to keep in mind when running code within the constructor is the following:
When you assign a variable to a newly created Object, two things will happen:
- The Object is instantiated
- The Variable is assigned to that Object.
They key here is that it happens in that exact order, Java will only assign the variable when the Object instantiation has fully finished. So by the time loadMetrics() is called, the variable has not been assigned yet.
One common practice is to just call #start() manually after initializing the object.
Removing the start() call from the constructor and changing the ProxyInitializeEvent handler to this for example:
@Subscribe
public void initialize(ProxyInitializeEvent event){
this.core = new OneVersionRemake(this);
this.core.start();
}would no longer violate that principle and should work as intended.
I hope this helped you understand why this happened a bit better and how to avoid mistakes like these in the future.
I tried my best to explain it :)
If that's the case then explain to me how the new way I have (which is returning a Map instead) works, despite me not changing anything in how the method is called?
It still calls core the same way as before but the NPE is gone and seems to actually work just fine, which is why I believe this to be an issue with how the bstats dependency was provided by the core itself and perhaps wasn't shaded/relocated in correctly.
Because you are no longer calling core.getPie() within loadMetrics().
Now you are passing a delegate instead, a Callable<String> to be precise.
The code inside this Callable is not touched during the method run, it is simply passed onto the Metrics code for later invocation.
Think of these functions this way: "Don't run this code, simply store it for later until I need it again". That is what passing this function as an argument does.
The code will still execute in the same order, but this portion:
core.getPieMap()won't be called until Metrics invokes this Callable which will first happen in the initial push from MetricsBase seen here:
https://github.com/Bastian/bStats-Metrics/blob/master/base/src/main/java/org/bstats/MetricsBase.java#L145-L148
and scheduled here:
https://github.com/Bastian/bStats-Metrics/blob/master/base/src/main/java/org/bstats/MetricsBase.java#L132-L134
So the code portion of core.getPieMap() will only be called ~3-6 minutes after the Server has started.
By then, the variable core will already have been assigned.
Hope that clears it up for you :)