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

Forge support #83

Open
wants to merge 24 commits into
base: dev/0.3.x
Choose a base branch
from
Open

Forge support #83

wants to merge 24 commits into from

Conversation

ExerciseBook
Copy link

@ExerciseBook ExerciseBook commented Jan 6, 2023

Notice Forge support is still under developing.

link to #40

Preview

Test on our FTB Stone Block3 server.
image

Something need to discuss

  1. Which forge version we should support? Now we are prepare to support 1.12 1.16 1.18, but we are not sure whether 1.19 can reuse 1.18's code. So it may add not less than 3 submodules @warmthdawn
  2. For compatibility, we downgrade gradle version and relocate more package. @ExerciseBook
  3. If we support many forge version, how the submodule naming.

Feel free to code review, we are trying to make it better.

@ExerciseBook ExerciseBook marked this pull request as draft January 6, 2023 15:52
@Cubxity
Copy link
Owner

Cubxity commented Jan 6, 2023

Great work, thanks for the contribution!

For compatibility, we downgrade gradle version and relocate more package.If we support many forge version, how the submodule naming.

Are there no other workarounds? I would ideally like to stick to the latest version of Gradle.

If we support many forge version, how the submodule naming.

forge-{minor version}, e.g. forge-18, forge-19

I will be reviewing the code later on.

@Cubxity
Copy link
Owner

Cubxity commented Jan 6, 2023

I recommend looking into https://github.com/architectury/architectury-loom instead of forgegradle. Forgegradle uses many Gradle internals, thus being incompatible with the latest version of Gradle.

@nicognaW
Copy link

nicognaW commented Jan 7, 2023

I recommend looking into https://github.com/architectury/architectury-loom instead of forgegradle. Forgegradle uses many Gradle internals, thus being incompatible with the latest version of Gradle.

I think @warmthdawn had tried architectury-loom (if you look through this commit), but it didn't work, so he refactored the plugin to forgegradle so it could build successfully.

We will continue to experiment with architecture-loom in the hopes that it will work.

@ExerciseBook
Copy link
Author

ExerciseBook commented Mar 27, 2023

image

After migrating to loom, It can work on my server

But ping count and login count is wrong.

@ExerciseBook ExerciseBook changed the title WIP: Forge support Forge support Mar 28, 2023
@ExerciseBook ExerciseBook marked this pull request as ready for review March 28, 2023 12:05
@ExerciseBook
Copy link
Author

if we set shadowjar after remap, manifest will be overwrite. We can only set mixin cfg manually.

@Cubxity
Copy link
Owner

Cubxity commented Apr 6, 2023

Is this PR ready for review? Are metric issues resolved yet?

@ExerciseBook
Copy link
Author

yes, this pr ready now
image

image

@ExerciseBook
Copy link
Author

@eaglesemanation If there is any problem when you try this branch, feel free to cantact me.

@eaglesemanation
Copy link

@ExerciseBook I'm not sure if I can actually build it, I'm not that familiar with minecraft mod development. Just really excited to get metrics on my forge server =)
Maybe I'll have some time on Sunday.

@eaglesemanation
Copy link

Last quarter was tiring, finally got some time to test this. Looks like it's working, tested on Vault Hunters server.
Screenshot from 2023-05-04 02-52-55
I'm not sure why TPS and MSPT graphs work in overview, but don't work in server section. That's probably my misconfiguration, same thing happens to my PaperMC server

@ExerciseBook
Copy link
Author

ExerciseBook commented May 5, 2023

@eaglesemanation Here is my TPS and MSPT expression. Maybe we are different in the metrics query.

TPS

rate(minecraft_tick_duration_seconds_count{instance=~"^$instance$",exported_job=~"^$job$"}[$__interval])

MSPT

histogram_quantile(0.99, sum(rate(minecraft_tick_duration_seconds_bucket{instance=~"^$instance$",exported_job=~"^$job$"}[$__interval])) by (le))
histogram_quantile(0.95, sum(rate(minecraft_tick_duration_seconds_bucket{instance=~"^$instance$",exported_job=~"^$job$"}[$__interval])) by (le))
histogram_quantile(0.75, sum(rate(minecraft_tick_duration_seconds_bucket{instance=~"^$instance$",exported_job=~"^$job$"}[$__interval])) by (le))
histogram_quantile(0.50, sum(rate(minecraft_tick_duration_seconds_bucket{instance=~"^$instance$",exported_job=~"^$job$"}[$__interval])) by (le))

https://grafana.com/grafana/dashboards/18584-unifiedmetrics-0-3-x-prometheus/?
I use this template.

@eaglesemanation
Copy link

Interesting, your queries did not work for me as well, but I found out the fix - replacing $__interval with $__rate_interval worked. I'm really new to Prometheus, so I have no clue if that's an actual fix, but at least it shows something =)

@mja00
Copy link

mja00 commented Jun 18, 2023

I've ran this in production for about a month now. 0 issues. Looks really good!

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.

None yet

6 participants