MetricsServer v2 #469

7ue5wu · 2023-07-03T04:31:02Z

Context

The result will show below, and I am still trying to flatten the measurement field. Wanna get some early feedback and advice.

`data: {"name":"httpServer-mantis-rxnetty-server-8487_requestBacklog","timestamp":1688354626236,"type":"GAUGE","model":"v2","measurement":"[Measurement{statistic='VALUE', value=0.0}]","tags":{}}

data: {"name":"httpServer-mantis-rxnetty-server-8487_failedRequests","timestamp":1688354626236,"type":"COUNTER","model":"v2","measurement":"[Measurement{statistic='COUNT', value=0.0}]","tags":{}}`

Checklist

./gradlew build compiles code correctly
Added new tests where applicable
./gradlew test passes all tests
Extended README or added javadocs where applicable

hmitnflx · 2023-07-07T21:38:52Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

- private Observable<Measurements> measurements(long timeFrequency) {
- final MetricsRegistry registry = MetricsRegistry.getInstance();
+ private Observable<MicrometerMeasurements> measurements(long timeFrequency) {
+ final MeterRegistry registry = Metrics.globalRegistry;


you can keep both.

final MeterRegistry micrometerRegistry = Metrics.globalRegistry; final MetricsRegistry mantisMeterRegistry = MetricsRegistry.getInstance();

hmitnflx · 2023-07-07T21:41:07Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

- measurements.add(new Measurements(metrics.getMetricGroupId().name(),
- timestamp, counters, gauges, tags));
+ List<MicrometerMeasurements> measurements = new ArrayList<>();
+ for (Meter meter: registry.getMeters()) {


for (Meter meter : mantisMeterRegistry) {
// ... keep everything the same. No changes!!!
}

for (Meter meter : micrometerRegistry) {
// ... do other thing
// we use Measurements.java and add a new field called List
}

hmitnflx · 2023-07-11T23:42:15Z

mantis-common/src/main/java/io/mantisrx/common/metrics/measurement/Measurements.java

@@ -46,6 +48,21 @@ public Measurements(
 this.tags = tags;
 }

+ @JsonCreator


instead of adding a new method, please add a new Collection<MicrometerMeasurements> arg to the original constructor and refactor accordingly.

hmitnflx · 2023-07-11T23:44:12Z

mantis-common/src/main/java/io/mantisrx/common/metrics/measurement/Measurements.java

 '}';
 }
+
+ public static class MicrometerMeasurement {


this is a good change, umm, minor thing if I may suggest is to create it as a separate class similar to GaugeMeasurement

hmitnflx

These changes look good to me, thank you for making these changes. Appreciate it!
I've asked @sundargates to also take a look at the PR for a second set of eyes.

hmitnflx · 2023-07-12T20:54:23Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

+ gauges.add(new GaugeMeasurement(gaugeEntry.getKey().metricName(), gaugeEntry.getValue().doubleValue()));
+ }
+ measurements.add(new Measurements(metrics.getMetricGroupId().name(),
+ timestamp, counters, gauges, null,tags));


minor: please use Collections.emptyList() instead of null

Already updated.

hmitnflx · 2023-07-12T20:54:29Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

+ for (Meter meter: micrometerregistry.getMeters()) {
+ Collection<MicrometerMeasurement> micrometers = new LinkedList<>();
+ micrometers.add(new MicrometerMeasurement(meter.getId().getType(), meter.measure().iterator().next().getValue()));
+ measurements.add(new Measurements(meter.getId().getName(), timestamp, null, null, micrometers, tags));


minor: please use Collections.emptyList() instead of null

Already updated.

sundargates · 2023-07-12T20:53:51Z

mantis-common/build.gradle

- compileOnly libraries.spectatorApi
+ implementation libraries.spectatorApi


Do we even need spectatorApi after migration to micrometer?

No, but I think we need to keep it now because we still keep both metrics during migration.

sundargates · 2023-07-12T20:54:12Z

mantis-common/build.gradle

@@ -41,6 +41,7 @@ dependencies {
 api libraries.slf4jLog4j12
 implementation libraries.commonsIo
 implementation 'net.jcip:jcip-annotations:1.0'
+ implementation 'io.micrometer:micrometer-core:1.11.0'


nit: move this to 3rdparty dependencies in root build.gradle.

Already updated.

sundargates · 2023-07-12T20:56:44Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

@@ -63,31 +67,39 @@ public MetricsServer(int port, long publishRateInSeconds, Map<String, String> ta
 }

 private Observable<Measurements> measurements(long timeFrequency) {
+ final MeterRegistry micrometerregistry = Metrics.globalRegistry;


nit: Can we avoid using this singleton?

Could you please be more specific?

sundargates · 2023-07-12T21:08:12Z

mantis-common/src/main/java/io/mantisrx/common/metrics/measurement/MicrometerMeasurement.java

+
+import io.micrometer.core.instrument.Meter;
+
+public class MicrometerMeasurement {


use lombok @value.

sundargates · 2023-07-12T21:18:15Z

mantis-common/src/main/java/io/mantisrx/common/metrics/measurement/MicrometerMeasurement.java

+
+import io.micrometer.core.instrument.Meter;
+
+public class MicrometerMeasurement {


Do we even need this type, or can we convert the micrometer measurements to the existing measures DS?

I think we should keep them separate to allow identify mantis metrics vs micrometer metrics.
Eventually, we can migrate Measurements to MicrometerMeasurement only.

sundargates · 2023-07-12T21:20:52Z

mantis-common/src/main/java/io/mantisrx/common/metrics/measurement/Measurements.java

@@ -38,11 +40,13 @@ public Measurements(
 @JsonProperty("timestamp") long timestamp,
 @JsonProperty("counters") Collection<CounterMeasurement> counters,
 @JsonProperty("gauges") Collection<GaugeMeasurement> gauges,
+ @JsonProperty("micrometers") Collection<MicrometerMeasurement> micrometers,


Why do we need this when we are already emitting the existing data-structures? Unless the downstream understands this DS, emitting it is unnecessary.

The CounterMeasurement and GaugeMeasurement are for existing mantis internal metrics. All micrometer metrics will be part of the MicrometerMeasurement for now.

hmitnflx

looks good to me except for reverting metrics change in MasterMain and other minor suggestions/comments.

I'll approve and we can merge after addressing

hmitnflx · 2023-07-18T00:10:04Z

...ol-plane/mantis-control-plane-server/src/main/java/io/mantisrx/server/master/MasterMain.java

- .addCounter("masterInitError")
- .build();
- Metrics m = MetricsRegistry.getInstance().registerAndGet(metrics);
+ String groupName = "MasterMain";


umm, I'd suggest not changing the metrics in this class to use micrometer just yet.

hmitnflx · 2023-07-18T00:17:50Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

 this.port = port;
 this.publishRateInSeconds = publishRateInSeconds;
 this.tags = tags;
 mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
 mapper.registerModule(new Jdk8Module());
+ this.registry = registry;


Suggested change

this.registry = registry;

this.micrometerRegistry = registry;

hmitnflx · 2023-07-18T00:17:59Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

@@ -53,41 +58,51 @@ public class MetricsServer {
 private int port;
 private Map<String, String> tags;
 private long publishRateInSeconds;
+ private MeterRegistry registry;


Suggested change

private MeterRegistry registry;

private MeterRegistry micrometerRegistry;

hmitnflx · 2023-07-18T00:18:11Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

 }

 private Observable<Measurements> measurements(long timeFrequency) {
+ final MeterRegistry micrometerregistry = registry;


Suggested change

final MeterRegistry micrometerregistry = registry;

hmitnflx · 2023-07-18T00:18:36Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

+ List<Measurements> measurements = new ArrayList<>();
+
+
+ for (Meter meter: micrometerregistry.getMeters()) {


Suggested change

for (Meter meter: micrometerregistry.getMeters()) {

for (Meter meter: this.micrometerRegistry.getMeters()) {

hmitnflx · 2023-07-18T00:19:02Z

...ol-plane/mantis-control-plane-server/src/main/java/io/mantisrx/server/master/MasterMain.java

@@ -117,17 +120,16 @@ public class MasterMain implements Service {
 private MasterConfiguration config;
 private SchedulingService schedulingService;
 private ILeadershipManager leadershipManager;
+ private final MeterRegistry registry;


Suggested change

private final MeterRegistry registry;

private final MeterRegistry micrometerRegistry;

hmitnflx · 2023-07-18T00:19:13Z

...ol-plane/mantis-control-plane-server/src/main/java/io/mantisrx/server/master/MasterMain.java

- public MasterMain(ConfigurationFactory configFactory, AuditEventSubscriber auditEventSubscriber) {
-
+ public MasterMain(ConfigurationFactory configFactory, AuditEventSubscriber auditEventSubscriber, MeterRegistry registry) {
+ this.registry = registry;


Suggested change

this.registry = registry;

this.micrometerRegistry = registry;

7ue5wu · 2023-07-18T00:45:30Z

looks good to me except for reverting metrics change in MasterMain and other minor suggestions/comments.

I'll approve and we can merge after addressing

Thank you, Harshit! Already updated.

hmitnflx

Thanks for making this change. I really appreciate it.
I believe it's your first change for mantis OSS project. Kudos!
I'll take care of merging it in sometime.

sundargates · 2023-07-20T06:16:42Z

mantis-common/build.gradle

@@ -41,6 +40,8 @@ dependencies {
 api libraries.slf4jLog4j12
 implementation libraries.commonsIo
 implementation 'net.jcip:jcip-annotations:1.0'
+ implementation libraries.micrometerCore
+ implementation libraries.spectatorApi


Is this needed because we are dual publishing the metrics?

Yes, the spectator library can be removed when all the metrics have been changed.

sundargates · 2023-07-20T06:26:31Z

mantis-common/src/main/java/io/mantisrx/common/metrics/MetricsServer.java

+ for (Meter meter: microRegistry.getMeters()) {
+ Collection<MicrometerMeasurement> micrometers = new LinkedList<>();
+ micrometers.add(new MicrometerMeasurement(meter.getId().getType(), meter.measure().iterator().next().getValue()));
+ measurements.add(new Measurements(meter.getId().getName(), timestamp, Collections.emptyList(), Collections.emptyList(), micrometers, tags));


We are generating a measurement instance for every micrometer metric, even though measurements are supposed to consist of a collection of counters, timers, and other components for a specific group ID. Considering this, is the semantics of having a 1:1 relationship between the measurements instance and micrometer metric correct?

Yes, it is correct.

sundargates · 2023-07-20T06:29:13Z

...ol-plane/mantis-control-plane-server/src/main/java/io/mantisrx/server/master/MasterMain.java

+ public MasterMain(ConfigurationFactory configFactory, AuditEventSubscriber auditEventSubscriber, MeterRegistry micrometerRegistry) {
+ this.micrometerRegistry = micrometerRegistry;


I think we can use the CompositeRegistry singleton here since this is the leaf node.

sundargates · 2023-07-20T06:30:00Z

mantis-runtime/src/main/java/io/mantisrx/runtime/executor/LocalJobExecutorNetworked.java

+ private final MeterRegistry registry;
+
+
+ private LocalJobExecutorNetworked(MeterRegistry registry) {


We don't need the MeterRegistry in the constructor here since this is also a leaf node. This would avoid some of the other extraneous changes.

sundargates

LGTM, except for a few minor comments.

…rement> in MetricsServer

… then add the arg

github-actions · 2023-09-05T20:25:32Z

Test Results

0 files - 128 0 suites - 128 0s ⏱️ - 7m 15s
0 tests - 545 0 ✔️ - 536 0 💤 - 8 0 ❌ - 1
0 runs - 546 0 ✔️ - 537 0 💤 - 8 0 ❌ - 1

Results for commit 32b27e3. ± Comparison against base commit 2b1ad2d.

♻️ This comment has been updated with latest results.

7ue5wu requested review from nickmahilani, jeffchao, piygoyal, calvin681, sundargates, Andyz26, hmitnflx, markcho, liuml07 and fdc-ntflx as code owners July 3, 2023 04:31

7ue5wu had a problem deploying to Integrate Pull Request July 3, 2023 04:31 — with GitHub Actions Failure

7ue5wu had a problem deploying to Integrate Pull Request July 10, 2023 00:42 — with GitHub Actions Failure

hmitnflx reviewed Jul 11, 2023

View reviewed changes

7ue5wu had a problem deploying to Integrate Pull Request July 12, 2023 00:17 — with GitHub Actions Failure

hmitnflx reviewed Jul 12, 2023

View reviewed changes

sundargates reviewed Jul 12, 2023

View reviewed changes

7ue5wu had a problem deploying to Integrate Pull Request July 13, 2023 14:53 — with GitHub Actions Failure

7ue5wu had a problem deploying to Integrate Pull Request July 15, 2023 21:17 — with GitHub Actions Failure

7ue5wu had a problem deploying to Integrate Pull Request July 17, 2023 22:48 — with GitHub Actions Failure

7ue5wu had a problem deploying to Integrate Pull Request July 18, 2023 00:09 — with GitHub Actions Failure

hmitnflx reviewed Jul 18, 2023

View reviewed changes

7ue5wu had a problem deploying to Integrate Pull Request July 18, 2023 00:32 — with GitHub Actions Failure

7ue5wu had a problem deploying to Integrate Pull Request July 18, 2023 00:42 — with GitHub Actions Failure

hmitnflx approved these changes Jul 18, 2023

View reviewed changes

sundargates reviewed Jul 20, 2023

View reviewed changes

sundargates approved these changes Jul 20, 2023

View reviewed changes

J. Wu and others added 3 commits September 5, 2023 13:23

MetricsServer v2

ce6aa9b

update MetricsServer v2 and MicrometerMeasurements

a24b046

update

75f783c

J. Wu added 9 commits September 5, 2023 13:23

add micrometers to Measurements and keep two version Observable<Measu…

bd17208

…rement> in MetricsServer

create separate MicrometerMeasurement class and delete the new method…

8468b75

… then add the arg

update as suggest

c5709a2

update dependencies

d012827

update registry

a631e0f

MetricsServerService and MasterMain

234ff55

LocalJobExecutorNetworked and SyntheticSourceJob

0fe7565

revert metrics in MasterMain

b0fef28

micrometerRegistry

49b2ab2

hmitnflx force-pushed the MetricsServer_v2 branch from eccb356 to 49b2ab2 Compare September 5, 2023 20:23

hmitnflx had a problem deploying to Integrate Pull Request September 5, 2023 20:24 — with GitHub Actions Failure

merge master

aa86f5d

hmitnflx had a problem deploying to Integrate Pull Request October 23, 2023 20:29 — with GitHub Actions Failure

build fixes

32b27e3

hmitnflx had a problem deploying to Integrate Pull Request October 23, 2023 21:07 — with GitHub Actions Failure

		compileOnly libraries.spectatorApi
		implementation libraries.spectatorApi


		import io.micrometer.core.instrument.Meter;

		public class MicrometerMeasurement {

	this.registry = registry;
	this.micrometerRegistry = registry;

	private MeterRegistry registry;
	private MeterRegistry micrometerRegistry;

		List<Measurements> measurements = new ArrayList<>();


		for (Meter meter: micrometerregistry.getMeters()) {

	for (Meter meter: micrometerregistry.getMeters()) {
	for (Meter meter: this.micrometerRegistry.getMeters()) {

	private final MeterRegistry registry;
	private final MeterRegistry micrometerRegistry;

		public MasterMain(ConfigurationFactory configFactory, AuditEventSubscriber auditEventSubscriber, MeterRegistry micrometerRegistry) {
		this.micrometerRegistry = micrometerRegistry;

		private final MeterRegistry registry;


		private LocalJobExecutorNetworked(MeterRegistry registry) {

MetricsServer v2 #469

Are you sure you want to change the base?

MetricsServer v2 #469

Conversation

7ue5wu commented Jul 3, 2023

Context

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmitnflx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmitnflx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7ue5wu commented Jul 18, 2023

hmitnflx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sundargates left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 5, 2023 • edited

Test Results

github-actions bot commented Sep 5, 2023 •

edited