Skip to content

Commit

Permalink
Extract repositories metrics into its own class (#103034)
Browse files Browse the repository at this point in the history
This PR is a follow up of
#102505 (comment)
that move the repositories metrics management into its own class which
is then passed around instead of relying on the raw meterRegistry and
string metric names.
  • Loading branch information
ywangd authored Dec 7, 2023
1 parent e20821f commit b9c2980
Show file tree
Hide file tree
Showing 42 changed files with 210 additions and 145 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.indices.recovery.RecoverySettings;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.blobstore.MeteredBlobStoreRepository;
import org.elasticsearch.telemetry.metric.MeterRegistry;
import org.elasticsearch.xcontent.NamedXContentRegistry;

import java.util.Locale;
Expand Down Expand Up @@ -109,7 +109,7 @@ public AzureRepository(
recoverySettings,
buildBasePath(metadata),
buildLocation(metadata),
MeterRegistry.NOOP
RepositoriesMetrics.NOOP
);
this.chunkSize = Repository.CHUNK_SIZE_SETTING.get(metadata.settings());
this.storageService = storageService;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.ReloadablePlugin;
import org.elasticsearch.plugins.RepositoryPlugin;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.Repository;
import org.elasticsearch.threadpool.ExecutorBuilder;
import org.elasticsearch.threadpool.ScalingExecutorBuilder;
Expand Down Expand Up @@ -62,7 +63,8 @@ public Map<String, Repository.Factory> getRepositories(
NamedXContentRegistry namedXContentRegistry,
ClusterService clusterService,
BigArrays bigArrays,
RecoverySettings recoverySettings
RecoverySettings recoverySettings,
RepositoriesMetrics repositoriesMetrics
) {
return Collections.singletonMap(AzureRepository.TYPE, metadata -> {
AzureStorageService storageService = azureStoreService.get();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.elasticsearch.env.Environment;
import org.elasticsearch.indices.recovery.RecoverySettings;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.RepositoriesService;
import org.elasticsearch.repositories.Repository;
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
Expand Down Expand Up @@ -256,7 +257,8 @@ public Map<String, Repository.Factory> getRepositories(
NamedXContentRegistry registry,
ClusterService clusterService,
BigArrays bigArrays,
RecoverySettings recoverySettings
RecoverySettings recoverySettings,
RepositoriesMetrics repositoriesMetrics
) {
return Collections.singletonMap(
GoogleCloudStorageRepository.TYPE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.ReloadablePlugin;
import org.elasticsearch.plugins.RepositoryPlugin;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.Repository;
import org.elasticsearch.xcontent.NamedXContentRegistry;

Expand Down Expand Up @@ -48,7 +49,8 @@ public Map<String, Repository.Factory> getRepositories(
NamedXContentRegistry namedXContentRegistry,
ClusterService clusterService,
BigArrays bigArrays,
RecoverySettings recoverySettings
RecoverySettings recoverySettings,
RepositoriesMetrics repositoriesMetrics
) {
return Collections.singletonMap(
GoogleCloudStorageRepository.TYPE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.indices.recovery.RecoverySettings;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.RepositoryException;
import org.elasticsearch.repositories.blobstore.MeteredBlobStoreRepository;
import org.elasticsearch.telemetry.metric.MeterRegistry;
import org.elasticsearch.xcontent.NamedXContentRegistry;

import java.util.Map;
Expand Down Expand Up @@ -78,7 +78,7 @@ class GoogleCloudStorageRepository extends MeteredBlobStoreRepository {
recoverySettings,
buildBasePath(metadata),
buildLocation(metadata),
MeterRegistry.NOOP
RepositoriesMetrics.NOOP
);
this.storageService = storageService;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@
import java.util.Queue;
import java.util.concurrent.LinkedBlockingQueue;

import static org.elasticsearch.repositories.RepositoriesModule.HTTP_REQUEST_TIME_IN_MICROS_HISTOGRAM;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_EXCEPTIONS_COUNT;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_EXCEPTIONS_HISTOGRAM;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_OPERATIONS_COUNT;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_REQUESTS_COUNT;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_THROTTLES_COUNT;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_THROTTLES_HISTOGRAM;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_UNSUCCESSFUL_OPERATIONS_COUNT;
import static org.elasticsearch.repositories.RepositoriesMetrics.HTTP_REQUEST_TIME_IN_MICROS_HISTOGRAM;
import static org.elasticsearch.repositories.RepositoriesMetrics.METRIC_EXCEPTIONS_COUNT;
import static org.elasticsearch.repositories.RepositoriesMetrics.METRIC_EXCEPTIONS_HISTOGRAM;
import static org.elasticsearch.repositories.RepositoriesMetrics.METRIC_OPERATIONS_COUNT;
import static org.elasticsearch.repositories.RepositoriesMetrics.METRIC_REQUESTS_COUNT;
import static org.elasticsearch.repositories.RepositoriesMetrics.METRIC_THROTTLES_COUNT;
import static org.elasticsearch.repositories.RepositoriesMetrics.METRIC_THROTTLES_HISTOGRAM;
import static org.elasticsearch.repositories.RepositoriesMetrics.METRIC_UNSUCCESSFUL_OPERATIONS_COUNT;
import static org.elasticsearch.rest.RestStatus.INTERNAL_SERVER_ERROR;
import static org.elasticsearch.rest.RestStatus.NOT_FOUND;
import static org.elasticsearch.rest.RestStatus.TOO_MANY_REQUESTS;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.elasticsearch.indices.recovery.RecoverySettings;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.PluginsService;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.RepositoriesService;
import org.elasticsearch.repositories.Repository;
import org.elasticsearch.repositories.RepositoryData;
Expand Down Expand Up @@ -74,7 +75,7 @@
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;

import static org.elasticsearch.repositories.RepositoriesModule.METRIC_REQUESTS_COUNT;
import static org.elasticsearch.repositories.RepositoriesMetrics.METRIC_REQUESTS_COUNT;
import static org.elasticsearch.repositories.blobstore.BlobStoreTestUtil.randomNonDataPurpose;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
Expand Down Expand Up @@ -444,9 +445,10 @@ protected S3Repository createRepository(
NamedXContentRegistry registry,
ClusterService clusterService,
BigArrays bigArrays,
RecoverySettings recoverySettings
RecoverySettings recoverySettings,
RepositoriesMetrics repositoriesMetrics
) {
return new S3Repository(metadata, registry, getService(), clusterService, bigArrays, recoverySettings, getMeterRegistry()) {
return new S3Repository(metadata, registry, getService(), clusterService, bigArrays, recoverySettings, repositoriesMetrics) {

@Override
public BlobStore blobStore() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.PluginsService;
import org.elasticsearch.repositories.AbstractThirdPartyRepositoryTestCase;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.RepositoriesService;
import org.elasticsearch.telemetry.metric.MeterRegistry;
import org.elasticsearch.test.ClusterServiceUtils;
import org.elasticsearch.test.fixtures.minio.MinioTestContainer;
import org.elasticsearch.test.fixtures.testcontainers.TestContainersThreadFilter;
Expand Down Expand Up @@ -145,7 +145,7 @@ public long absoluteTimeInMillis() {
ClusterServiceUtils.createClusterService(threadpool),
BigArrays.NON_RECYCLING_INSTANCE,
new RecoverySettings(node().settings(), node().injector().getInstance(ClusterService.class).getClusterSettings()),
MeterRegistry.NOOP
RepositoriesMetrics.NOOP
)
) {
repository.start();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.telemetry.metric.LongCounter;
import org.elasticsearch.telemetry.metric.LongHistogram;
import org.elasticsearch.telemetry.metric.MeterRegistry;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.threadpool.ThreadPool;

import java.io.IOException;
Expand All @@ -54,14 +52,6 @@
import java.util.stream.Collectors;

import static org.elasticsearch.core.Strings.format;
import static org.elasticsearch.repositories.RepositoriesModule.HTTP_REQUEST_TIME_IN_MICROS_HISTOGRAM;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_EXCEPTIONS_COUNT;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_EXCEPTIONS_HISTOGRAM;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_OPERATIONS_COUNT;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_REQUESTS_COUNT;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_THROTTLES_COUNT;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_THROTTLES_HISTOGRAM;
import static org.elasticsearch.repositories.RepositoriesModule.METRIC_UNSUCCESSFUL_OPERATIONS_COUNT;

class S3BlobStore implements BlobStore {

Expand Down Expand Up @@ -91,15 +81,7 @@ class S3BlobStore implements BlobStore {

private final ThreadPool threadPool;
private final Executor snapshotExecutor;
private final MeterRegistry meterRegistry;
private final LongCounter requestCounter;
private final LongCounter exceptionCounter;
private final LongCounter throttleCounter;
private final LongCounter operationCounter;
private final LongCounter unsuccessfulOperationCounter;
private final LongHistogram exceptionHistogram;
private final LongHistogram throttleHistogram;
private final LongHistogram httpRequestTimeInMicroHistogram;
private final RepositoriesMetrics repositoriesMetrics;

private final StatsCollectors statsCollectors = new StatsCollectors();

Expand All @@ -117,7 +99,7 @@ class S3BlobStore implements BlobStore {
RepositoryMetadata repositoryMetadata,
BigArrays bigArrays,
ThreadPool threadPool,
MeterRegistry meterRegistry
RepositoriesMetrics repositoriesMetrics
) {
this.service = service;
this.bigArrays = bigArrays;
Expand All @@ -129,15 +111,7 @@ class S3BlobStore implements BlobStore {
this.repositoryMetadata = repositoryMetadata;
this.threadPool = threadPool;
this.snapshotExecutor = threadPool.executor(ThreadPool.Names.SNAPSHOT);
this.meterRegistry = meterRegistry;
this.requestCounter = this.meterRegistry.getLongCounter(METRIC_REQUESTS_COUNT);
this.exceptionCounter = this.meterRegistry.getLongCounter(METRIC_EXCEPTIONS_COUNT);
this.throttleCounter = this.meterRegistry.getLongCounter(METRIC_THROTTLES_COUNT);
this.operationCounter = this.meterRegistry.getLongCounter(METRIC_OPERATIONS_COUNT);
this.unsuccessfulOperationCounter = this.meterRegistry.getLongCounter(METRIC_UNSUCCESSFUL_OPERATIONS_COUNT);
this.exceptionHistogram = this.meterRegistry.getLongHistogram(METRIC_EXCEPTIONS_HISTOGRAM);
this.throttleHistogram = this.meterRegistry.getLongHistogram(METRIC_THROTTLES_HISTOGRAM);
this.httpRequestTimeInMicroHistogram = this.meterRegistry.getLongHistogram(HTTP_REQUEST_TIME_IN_MICROS_HISTOGRAM);
this.repositoriesMetrics = repositoriesMetrics;
s3RequestRetryStats = new S3RequestRetryStats(getMaxRetries());
threadPool.scheduleWithFixedDelay(() -> {
var priorRetryStats = s3RequestRetryStats;
Expand Down Expand Up @@ -214,21 +188,21 @@ public final void collectMetrics(Request<?> request, Response<?> response) {
.map(List::size)
.orElse(0);

operationCounter.incrementBy(1, attributes);
repositoriesMetrics.operationCounter().incrementBy(1, attributes);
if (numberOfAwsErrors == requestCount) {
unsuccessfulOperationCounter.incrementBy(1, attributes);
repositoriesMetrics.unsuccessfulOperationCounter().incrementBy(1, attributes);
}

requestCounter.incrementBy(requestCount, attributes);
repositoriesMetrics.requestCounter().incrementBy(requestCount, attributes);
if (exceptionCount > 0) {
exceptionCounter.incrementBy(exceptionCount, attributes);
exceptionHistogram.record(exceptionCount, attributes);
repositoriesMetrics.exceptionCounter().incrementBy(exceptionCount, attributes);
repositoriesMetrics.exceptionHistogram().record(exceptionCount, attributes);
}
if (throttleCount > 0) {
throttleCounter.incrementBy(throttleCount, attributes);
throttleHistogram.record(throttleCount, attributes);
repositoriesMetrics.throttleCounter().incrementBy(throttleCount, attributes);
repositoriesMetrics.throttleHistogram().record(throttleCount, attributes);
}
httpRequestTimeInMicroHistogram.record(getHttpRequestTimeInMicros(request), attributes);
repositoriesMetrics.httpRequestTimeInMicroHistogram().record(getHttpRequestTimeInMicros(request), attributes);
}

private boolean assertConsistencyBetweenHttpRequestAndOperation(Request<?> request, Operation operation) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@
import org.elasticsearch.indices.recovery.RecoverySettings;
import org.elasticsearch.monitor.jvm.JvmInfo;
import org.elasticsearch.repositories.FinalizeSnapshotContext;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.RepositoryData;
import org.elasticsearch.repositories.RepositoryException;
import org.elasticsearch.repositories.blobstore.MeteredBlobStoreRepository;
import org.elasticsearch.snapshots.SnapshotDeleteListener;
import org.elasticsearch.snapshots.SnapshotsService;
import org.elasticsearch.telemetry.metric.MeterRegistry;
import org.elasticsearch.threadpool.Scheduler;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xcontent.NamedXContentRegistry;
Expand Down Expand Up @@ -205,7 +205,7 @@ class S3Repository extends MeteredBlobStoreRepository {
final ClusterService clusterService,
final BigArrays bigArrays,
final RecoverySettings recoverySettings,
final MeterRegistry meterRegistry
final RepositoriesMetrics repositoriesMetrics
) {
super(
metadata,
Expand All @@ -215,7 +215,7 @@ class S3Repository extends MeteredBlobStoreRepository {
recoverySettings,
buildBasePath(metadata),
buildLocation(metadata),
meterRegistry
repositoriesMetrics
);
this.service = service;
this.snapshotExecutor = threadPool().executor(ThreadPool.Names.SNAPSHOT);
Expand Down Expand Up @@ -408,7 +408,7 @@ protected S3BlobStore createBlobStore() {
metadata,
bigArrays,
threadPool,
meterRegistry
repositoriesMetrics
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.ReloadablePlugin;
import org.elasticsearch.plugins.RepositoryPlugin;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.Repository;
import org.elasticsearch.telemetry.metric.MeterRegistry;
import org.elasticsearch.xcontent.NamedXContentRegistry;

import java.io.IOException;
Expand Down Expand Up @@ -60,7 +60,6 @@ public class S3RepositoryPlugin extends Plugin implements RepositoryPlugin, Relo
}

private final SetOnce<S3Service> service = new SetOnce<>();
private final SetOnce<MeterRegistry> meterRegistry = new SetOnce<>();
private final Settings settings;

public S3RepositoryPlugin(Settings settings) {
Expand All @@ -77,16 +76,16 @@ protected S3Repository createRepository(
final NamedXContentRegistry registry,
final ClusterService clusterService,
final BigArrays bigArrays,
final RecoverySettings recoverySettings
final RecoverySettings recoverySettings,
final RepositoriesMetrics repositoriesMetrics
) {
return new S3Repository(metadata, registry, service.get(), clusterService, bigArrays, recoverySettings, meterRegistry.get());
return new S3Repository(metadata, registry, service.get(), clusterService, bigArrays, recoverySettings, repositoriesMetrics);
}

@Override
public Collection<?> createComponents(PluginServices services) {
service.set(s3Service(services.environment(), services.clusterService().getSettings()));
this.service.get().refreshAndClearCache(S3ClientSettings.load(settings));
meterRegistry.set(services.telemetryProvider().getMeterRegistry());
return List.of(service);
}

Expand All @@ -100,11 +99,12 @@ public Map<String, Repository.Factory> getRepositories(
final NamedXContentRegistry registry,
final ClusterService clusterService,
final BigArrays bigArrays,
final RecoverySettings recoverySettings
final RecoverySettings recoverySettings,
RepositoriesMetrics repositoriesMetrics
) {
return Collections.singletonMap(
S3Repository.TYPE,
metadata -> createRepository(metadata, registry, clusterService, bigArrays, recoverySettings)
metadata -> createRepository(metadata, registry, clusterService, bigArrays, recoverySettings, repositoriesMetrics)
);
}

Expand Down Expand Up @@ -146,8 +146,4 @@ public void reload(Settings settings) {
public void close() throws IOException {
getService().close();
}

protected MeterRegistry getMeterRegistry() {
return meterRegistry.get();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@
import org.elasticsearch.indices.recovery.RecoverySettings;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.PluginsService;
import org.elasticsearch.repositories.RepositoriesMetrics;
import org.elasticsearch.repositories.RepositoriesService;
import org.elasticsearch.rest.AbstractRestChannel;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.RestResponse;
import org.elasticsearch.rest.action.admin.cluster.RestGetRepositoriesAction;
import org.elasticsearch.telemetry.metric.MeterRegistry;
import org.elasticsearch.test.ESSingleNodeTestCase;
import org.elasticsearch.test.rest.FakeRestRequest;
import org.elasticsearch.xcontent.NamedXContentRegistry;
Expand Down Expand Up @@ -262,9 +262,10 @@ protected S3Repository createRepository(
NamedXContentRegistry registry,
ClusterService clusterService,
BigArrays bigArrays,
RecoverySettings recoverySettings
RecoverySettings recoverySettings,
RepositoriesMetrics repositoriesMetrics
) {
return new S3Repository(metadata, registry, getService(), clusterService, bigArrays, recoverySettings, MeterRegistry.NOOP) {
return new S3Repository(metadata, registry, getService(), clusterService, bigArrays, recoverySettings, repositoriesMetrics) {
@Override
protected void assertSnapshotOrGenericThread() {
// eliminate thread name check as we create repo manually on test/main threads
Expand Down
Loading

0 comments on commit b9c2980

Please sign in to comment.