yifan-c commented on code in PR #204:
URL: https://github.com/apache/cassandra-sidecar/pull/204#discussion_r1984235321
##########
server/src/main/java/org/apache/cassandra/sidecar/datahub/IdentifiersProvider.java:
##########
@@ -231,13 +251,19 @@ public boolean equals(@Nullable Object other)
@NotNull
public String toString()
{
- return new ToStringBuilder(this)
+ return new ToStringBuilder(this, STYLE)
.append(this.organization())
.append(this.platform())
.append(this.environment())
.append(this.application())
.append(this.cluster())
.append(this.identifier())
- .toString();
+ .toString()
+ .replaceAll("\\s", "");
Review Comment:
The toString is relatively complex. Is there a test?
##########
server/src/main/java/org/apache/cassandra/sidecar/datahub/SchemaReporter.java:
##########
@@ -96,45 +110,73 @@ public SchemaReporter(@NotNull IdentifiersProvider
identifiersProvider,
* @param keyspaceConverters a {@link List} of {@link
KeyspaceToAspectConverter} instances to use
* @param tableConverters a {@link List} of {@link TableToAspectConverter}
instances to use
* @param emitterFactory an instance of {@link EmitterFactory} to use
+ * @param reportingMetrics an instance of {@link SchemaReportingMetrics}
to use
*/
protected SchemaReporter(@NotNull IdentifiersProvider identifiersProvider,
@NotNull List<ClusterToAspectConverter<? extends
RecordTemplate>> clusterConverters,
@NotNull List<KeyspaceToAspectConverter<? extends
RecordTemplate>> keyspaceConverters,
@NotNull List<TableToAspectConverter<? extends
RecordTemplate>> tableConverters,
- @NotNull EmitterFactory emitterFactory)
+ @NotNull EmitterFactory emitterFactory,
+ @NotNull SchemaReportingMetrics reportingMetrics)
{
this.identifiersProvider = identifiersProvider;
this.clusterConverters = clusterConverters;
this.keyspaceConverters = keyspaceConverters;
this.tableConverters = tableConverters;
this.emitterFactory = emitterFactory;
+ this.reportingMetrics = reportingMetrics;
}
/**
- * Public method for converting and reporting the Cassandra schema
+ * Public method for converting and reporting the Cassandra schema when
triggered by a scheduled periodic task
*
* @param cluster the {@link Cluster} to extract Cassandra schema from
*/
- public void process(@NotNull Cluster cluster)
+ public void processScheduled(@NotNull Cluster cluster)
{
- process(cluster.getMetadata());
+ process(cluster.getMetadata(),
reportingMetrics.startedSchedule.metric);
}
/**
- * Public method for converting and reporting the Cassandra schema
+ * Public method for converting and reporting the Cassandra schema when
triggered by a received API request
*
* @param metadata the {@link Metadata} to extract Cassandra schema from
*/
- public void process(@NotNull Metadata metadata)
+ public void processRequested(@NotNull Metadata metadata)
{
+ process(metadata, reportingMetrics.startedRequest.metric);
+ }
+
+ /**
+ * Private method for converting and reporting the Cassandra schema
+ *
+ * @param metadata the {@link Metadata} to extract Cassandra schema from
+ * @param started the {@link DeltaGauge} for the metric counting
invocations
+ */
+ private void process(@NotNull Metadata metadata,
+ @NotNull DeltaGauge started)
+ {
+ String action = " reporting schema for cluster with identifiers " +
identifiersProvider;
+ LOGGER.info("Started" + action);
Review Comment:
Use the value placeholder of logger to eval lazily. Update the other places
too.
```suggestion
LOGGER.info("Started {}", action);
```
##########
server/src/main/java/org/apache/cassandra/sidecar/metrics/server/SchemaReportingMetrics.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.metrics.server;
+
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.MetricRegistry;
+import org.apache.cassandra.sidecar.metrics.DeltaGauge;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+import org.apache.cassandra.sidecar.metrics.ServerMetrics;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Tracks metrics for the schema reporting done by Sidecar
+ */
+public class SchemaReportingMetrics
+{
+ protected static final String DOMAIN = ServerMetrics.SERVER_PREFIX +
".SchemaReporting";
+ protected static final String STARTED = "Started";
+ protected static final String FINISHED = "Finished";
+ protected static final String DURATION = "Duration";
+ protected static final String SIZE = "Size";
+ protected static final String TRIGGER = "Trigger";
+ protected static final String RESULT = "Result";
+ protected static final String UNIT = "Unit";
+ protected static final String REQUEST = "Request";
+ protected static final String SCHEDULE = "Schedule";
+ protected static final String SUCCESS = "Success";
+ protected static final String FAILURE = "Failure";
+ protected static final String ASPECTS = "Aspects";
+ protected static final String MILLISECONDS = "Milliseconds";
+
+ public final NamedMetric<DeltaGauge> startedRequest;
+ public final NamedMetric<DeltaGauge> startedSchedule;
+ public final NamedMetric<DeltaGauge> finishedSuccess;
+ public final NamedMetric<DeltaGauge> finishedFailure;
+ public final NamedMetric<Histogram> durationMilliseconds;
+ public final NamedMetric<Histogram> sizeAspects;
Review Comment:
Please update the patch to follow the other Metrics class? Let them be
consistent. The string constants that are _only used once_ can all be removed.
##########
server/src/test/integration/org/apache/cassandra/sidecar/datahub/SchemaReporterIntegrationTest.java:
##########
@@ -97,21 +124,31 @@ void testSchemaConverter() throws IOException
JsonEmitter emitter = new JsonEmitter();
try (Session session = maybeGetSession())
{
- new SchemaReporter(IDENTIFIERS, () -> emitter)
- .process(session.getCluster());
+ new SchemaReporter(IDENTIFIERS, () -> emitter, metrics)
+ .processScheduled(session.getCluster());
}
String actualJson = normalizeNames(emitter.content());
String expectedJson =
IOUtils.readFully("/datahub/integration_test.json");
-
assertThat(actualJson)
.isEqualToNormalizingWhitespace(expectedJson);
- // Finally, make sure the returned schema produces the same tree of
+ // Second, make sure the returned schema produces the same tree of
// DataHub objects after having been normalized and deserialized
DataList actualData = CODEC.readList(new StringReader(actualJson));
DataList expectedData = CODEC.readList(new StringReader(expectedJson));
-
assertThat(actualData)
.isEqualTo(expectedData);
+
+ // Third, validate the captured metrics: one execution triggered by
the schedule and
+ // completed successfully, with thirteen aspects produced in zero or
more milliseconds
+ SchemaReportingMetrics metrics =
this.metrics.server().schemaReporting();
+ assertEquals(0L, metrics.startedRequest.metric.getValue());
+ assertEquals(1L, metrics.startedSchedule.metric.getValue());
+ assertEquals(1L, metrics.finishedSuccess.metric.getValue());
+ assertEquals(0L, metrics.finishedFailure.metric.getValue());
+ assertEquals(1L, metrics.sizeAspects.metric.getCount());
+ assertEquals(13L,
metrics.sizeAspects.metric.getSnapshot().getValues()[0]);
+ assertEquals(1L, metrics.durationMilliseconds.metric.getCount());
+ assertTrue(0L <=
metrics.durationMilliseconds.metric.getSnapshot().getValues()[0]);
Review Comment:
Use assertJ
##########
server/src/main/java/org/apache/cassandra/sidecar/datahub/SchemaReporter.java:
##########
@@ -96,45 +110,73 @@ public SchemaReporter(@NotNull IdentifiersProvider
identifiersProvider,
* @param keyspaceConverters a {@link List} of {@link
KeyspaceToAspectConverter} instances to use
* @param tableConverters a {@link List} of {@link TableToAspectConverter}
instances to use
* @param emitterFactory an instance of {@link EmitterFactory} to use
+ * @param reportingMetrics an instance of {@link SchemaReportingMetrics}
to use
*/
protected SchemaReporter(@NotNull IdentifiersProvider identifiersProvider,
@NotNull List<ClusterToAspectConverter<? extends
RecordTemplate>> clusterConverters,
@NotNull List<KeyspaceToAspectConverter<? extends
RecordTemplate>> keyspaceConverters,
@NotNull List<TableToAspectConverter<? extends
RecordTemplate>> tableConverters,
- @NotNull EmitterFactory emitterFactory)
+ @NotNull EmitterFactory emitterFactory,
+ @NotNull SchemaReportingMetrics reportingMetrics)
{
this.identifiersProvider = identifiersProvider;
this.clusterConverters = clusterConverters;
this.keyspaceConverters = keyspaceConverters;
this.tableConverters = tableConverters;
this.emitterFactory = emitterFactory;
+ this.reportingMetrics = reportingMetrics;
}
/**
- * Public method for converting and reporting the Cassandra schema
+ * Public method for converting and reporting the Cassandra schema when
triggered by a scheduled periodic task
*
* @param cluster the {@link Cluster} to extract Cassandra schema from
*/
- public void process(@NotNull Cluster cluster)
+ public void processScheduled(@NotNull Cluster cluster)
{
- process(cluster.getMetadata());
+ process(cluster.getMetadata(),
reportingMetrics.startedSchedule.metric);
}
/**
- * Public method for converting and reporting the Cassandra schema
+ * Public method for converting and reporting the Cassandra schema when
triggered by a received API request
*
* @param metadata the {@link Metadata} to extract Cassandra schema from
*/
- public void process(@NotNull Metadata metadata)
+ public void processRequested(@NotNull Metadata metadata)
{
+ process(metadata, reportingMetrics.startedRequest.metric);
+ }
+
+ /**
+ * Private method for converting and reporting the Cassandra schema
+ *
+ * @param metadata the {@link Metadata} to extract Cassandra schema from
+ * @param started the {@link DeltaGauge} for the metric counting
invocations
+ */
+ private void process(@NotNull Metadata metadata,
+ @NotNull DeltaGauge started)
+ {
+ String action = " reporting schema for cluster with identifiers " +
identifiersProvider;
Review Comment:
you can remove the heading space. It should make the error message in the
RuntimeException look a bit nicer too.
##########
server/src/test/java/org/apache/cassandra/sidecar/datahub/SchemaReporterTest.java:
##########
@@ -107,20 +141,29 @@ void testEmptyTable() throws IOException
when(options.getComment()).thenReturn("table comment");
JsonEmitter emitter = new JsonEmitter();
- new SchemaReporter(IDENTIFIERS, () -> emitter)
- .process(cluster);
+ new SchemaReporter(IDENTIFIERS, () -> emitter, metrics)
+ .processRequested(metadata);
String actual = emitter.content();
String expected = IOUtils.readFully("/datahub/empty_table.json");
-
assertEquals(expected, actual);
+
+ SchemaReportingMetrics metrics =
this.metrics.server().schemaReporting(); // Validate captured
metrics:
+ assertEquals(1L, metrics.startedRequest.metric.getValue());
// * one execution triggered by request
+ assertEquals(0L, metrics.startedSchedule.metric.getValue());
// * zero executions triggered by schedule
+ assertEquals(1L, metrics.finishedSuccess.metric.getValue());
// * one execution resulted in success
+ assertEquals(0L, metrics.finishedFailure.metric.getValue());
// * zero executions resulted in failure
+ assertEquals(1L, metrics.sizeAspects.metric.getCount());
// * single number of aspects,
+ assertEquals(13L,
metrics.sizeAspects.metric.getSnapshot().getValues()[0]); // equal
to thirteen
+ assertEquals(1L, metrics.durationMilliseconds.metric.getCount());
// * single duration of execution,
+ assertTrue(0L <=
metrics.durationMilliseconds.metric.getSnapshot().getValues()[0]); // that
is non-negative
Review Comment:
use assertJ
##########
server/src/main/java/org/apache/cassandra/sidecar/metrics/server/SchemaReportingMetrics.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.metrics.server;
+
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.MetricRegistry;
+import org.apache.cassandra.sidecar.metrics.DeltaGauge;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+import org.apache.cassandra.sidecar.metrics.ServerMetrics;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Tracks metrics for the schema reporting done by Sidecar
+ */
+public class SchemaReportingMetrics
+{
+ protected static final String DOMAIN = ServerMetrics.SERVER_PREFIX +
".SchemaReporting";
+ protected static final String STARTED = "Started";
+ protected static final String FINISHED = "Finished";
+ protected static final String DURATION = "Duration";
+ protected static final String SIZE = "Size";
+ protected static final String TRIGGER = "Trigger";
+ protected static final String RESULT = "Result";
+ protected static final String UNIT = "Unit";
+ protected static final String REQUEST = "Request";
+ protected static final String SCHEDULE = "Schedule";
+ protected static final String SUCCESS = "Success";
+ protected static final String FAILURE = "Failure";
+ protected static final String ASPECTS = "Aspects";
+ protected static final String MILLISECONDS = "Milliseconds";
+
+ public final NamedMetric<DeltaGauge> startedRequest;
+ public final NamedMetric<DeltaGauge> startedSchedule;
+ public final NamedMetric<DeltaGauge> finishedSuccess;
+ public final NamedMetric<DeltaGauge> finishedFailure;
+ public final NamedMetric<Histogram> durationMilliseconds;
+ public final NamedMetric<Histogram> sizeAspects;
+
+ public SchemaReportingMetrics(@NotNull MetricRegistry registry)
+ {
+ startedSchedule = NamedMetric.builder(name -> registry.gauge(name,
DeltaGauge::new))
+ .withDomain(DOMAIN)
+ .withName(STARTED)
+ .addTag(TRIGGER, SCHEDULE)
+ .build();
+ startedRequest = NamedMetric.builder(name -> registry.gauge(name,
DeltaGauge::new))
+ .withDomain(DOMAIN)
+ .withName(STARTED)
+ .addTag(TRIGGER, REQUEST)
+ .build();
+
+ finishedSuccess = NamedMetric.builder(name -> registry.gauge(name,
DeltaGauge::new))
+ .withDomain(DOMAIN)
+ .withName(FINISHED)
+ .addTag(RESULT, SUCCESS)
+ .build();
+ finishedFailure = NamedMetric.builder(name -> registry.gauge(name,
DeltaGauge::new))
+ .withDomain(DOMAIN)
+ .withName(FINISHED)
+ .addTag(RESULT, FAILURE)
+ .build();
+
+ sizeAspects = NamedMetric.builder(registry::histogram)
+ .withDomain(DOMAIN)
+ .withName(SIZE)
+ .addTag(UNIT, ASPECTS)
+ .build();
+
+ durationMilliseconds = NamedMetric.builder(registry::histogram)
+ .withDomain(DOMAIN)
+ .withName(DURATION)
+ .addTag(UNIT, MILLISECONDS)
+ .build();
Review Comment:
Prefer self-describing names over tags. For example, "StartedBySchedule"
provides better readability than "Trigger=Schedule.Started". Note that all the
tags are combined into this `key=value` format at the end.
##########
server/src/test/java/org/apache/cassandra/sidecar/datahub/SchemaReporterTest.java:
##########
@@ -261,12 +311,21 @@ void testUserTypes() throws IOException
when(udt2c2.getType()).thenReturn(DataType.cboolean());
JsonEmitter emitter = new JsonEmitter();
- new SchemaReporter(IDENTIFIERS, () -> emitter)
- .process(cluster);
+ new SchemaReporter(IDENTIFIERS, () -> emitter, metrics)
+ .processScheduled(cluster);
String actual = emitter.content();
String expected = IOUtils.readFully("/datahub/user_types.json");
-
assertEquals(expected, actual);
+
+ SchemaReportingMetrics metrics =
this.metrics.server().schemaReporting(); // Validate captured
metrics:
+ assertEquals(0L, metrics.startedRequest.metric.getValue());
// * zero executions triggered by request
+ assertEquals(1L, metrics.startedSchedule.metric.getValue());
// * one execution triggered by schedule
+ assertEquals(1L, metrics.finishedSuccess.metric.getValue());
// * one execution resulted in success
+ assertEquals(0L, metrics.finishedFailure.metric.getValue());
// * zero executions resulted in failure
+ assertEquals(1L, metrics.sizeAspects.metric.getCount());
// * single number of aspects,
+ assertEquals(13L,
metrics.sizeAspects.metric.getSnapshot().getValues()[0]); // equal
to thirteen
+ assertEquals(1L, metrics.durationMilliseconds.metric.getCount());
// * single duration of execution,
+ assertTrue(0L <=
metrics.durationMilliseconds.metric.getSnapshot().getValues()[0]); // that
is non-negative
Review Comment:
Use assertJ
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
