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

feat(MonitoringSubsystem): provide Micrometer gauges for rendering.world #4950

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

keturn
Copy link
Member

@keturn keturn commented Nov 15, 2021

I started looking at sending the stats available on the DebugOverlay screens to Micrometer, and this happened.

Conceptually, this is working in the area of MetricsMode and DebugMetricsSystem, but when I saw those were all relying on formatted strings with comments like this, it was clear I wouldn't be able to use those classes as they are now.

* @return a string containing metrics labels and associated values, separated by new lines.
*/
// TODO: transform this to return an object or a map. Consumers would then take care of formatting.
String getMetrics();

Example of Defining Meters

public static final List<GaugeMapEntry> GAUGE_MAP = List.of(
new GaugeMapEntry(WorldRenderer.class,
new GaugeSpec<WorldRendererImpl>(
PREFIX + ".emptyMeshChunks",
"Empty Mesh Chunks",
wri -> wri.statChunkMeshEmpty,
BaseUnits.OBJECTS),
new GaugeSpec<WorldRendererImpl>(
PREFIX + ".unreadyChunks",
"Unready Chunks",
wri -> wri.statChunkNotReady,
BaseUnits.OBJECTS),
new GaugeSpec<WorldRendererImpl>(
PREFIX + ".triangles",
"Rendered Triangles",
wri -> wri.statRenderedTriangles,
BaseUnits.OBJECTS)
),
new GaugeMapEntry(RenderableWorld.class,
new GaugeSpec<RenderableWorldImpl>(
PREFIX + ".visibleChunks",
"Visible Chunks",
rwi -> rwi.statVisibleChunks,
BaseUnits.OBJECTS),
new GaugeSpec<RenderableWorldImpl>(
PREFIX + ".dirtyChunks",
"Dirty Chunks",
rwi -> rwi.statDirtyChunks,
BaseUnits.OBJECTS),
new GaugeSpec<RenderableWorldImpl>(
PREFIX + ".dirtyChunks",
"Ignored Phases",
rwi -> rwi.statIgnoredPhases)
)
);

Notable qualities of this:

  • To avoid the need to add Micrometer-specific stuff in to all our current classes, the meters are defined in a separate class. But it can be in the same package to allow them to access package-private variables.
  • Definitions are grouped by the interface the object is registered under in the Context, but a definition may be for a more specific implementation of that interface.
  • We can have these definitions available without connecting them to live instances and publishers.

Other miscellania:

  • The use of Flux here isn't for anything asynchronous. I switched from Stream because Flux has a few convenient processing methods that Stream lacks, such as doOnDiscard and mapNotNull.

Future considerations:

  • There are other kinds of meters besides Gauges. We're sure to use Timers, and probably Counters too. They have the same metadata as gauges (name, description, tags, etc) but they have some state that gauges don't, so there will be some differences in how we set those up.

How to test

Brief description of how to test / confirm this PR before merging

Outstanding before merging

  • migrate WorldRendererMode to read from the meter registry.
  • could use translation work, but we'll probably consider these to be more like logger calls than user-facing, and skip them.

@keturn keturn added Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Status: Needs Discussion Requires help discussing a reported issue or provided PR Type: Improvement Request for or addition/enhancement of a feature labels Nov 15, 2021
@keturn
Copy link
Member Author

keturn commented Nov 15, 2021

Oh no, I thought I had finally reached agreement with the type system, but I checked in something that doesn't compile?

I was thinking that the method signatures ended up suspiciously uncluttered in my latest version. Will re-examine tomorrow. 🤞

@keturn
Copy link
Member Author

keturn commented Nov 15, 2021

Okay, now it compiles and initializes correctly. 😌

Base automatically changed from spike/jmx to develop December 19, 2021 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Discussion Requires help discussing a reported issue or provided PR Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Type: Improvement Request for or addition/enhancement of a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant