Differentiate stats for the same blobstore operation with purposes #99615

ywangd · 2023-09-18T05:09:22Z

Today blobstore stats are collected against each HTTP operation, e.g. Get, List. This is not granular enough because the same HTTP operration can be performed for different purposes, e.g. cluster state, indices or translog. This PR adds a new Purpose enum to provide further breakdown for the same HTTP operation.

Relates: ES-6800

DaveCTurner · 2023-09-18T07:23:33Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java

+    @Nullable
+    private final Purpose purpose;


I would much prefer this not to be nullable, let's represent the cases that would be null as a proper enum value instead. Indeed at some point in the future I think we'll want to distinguish metadata and data operations too, and this seems like a good mechanism for that.

Thanks David. I replaced null with Purpose.Generic in 0a39942

DaveCTurner · 2023-09-18T07:26:24Z

server/src/main/java/org/elasticsearch/common/blobstore/BlobStore.java

@@ -29,6 +32,14 @@ public interface BlobStore extends Closeable {
     */
    void deleteBlobsIgnoringIfNotExists(Iterator<String> blobNames) throws IOException;

+    default void deleteBlobsIgnoringIfNotExists(Iterator<String> blobNames, @Nullable Purpose purpose) throws IOException {


I would also much prefer not to have an implicit default for the purpose parameter like this. Experience shows that we will forget to set it in future work unless the caller is required to make an explicit choice.

That said, temporarily adding an implicit default is a reasonable strategy for avoiding breaking the serverless build, but it must only be temporary. Ideally we'd mark the overloads that don't take the Purpose as @Deprecated(forRemoval = true).

Agreed especially since this interface method is pretty new. I will go through the deprecation and removal process once we are happy with the proposed approach in this PR.

elasticsearchmachine · 2023-09-20T02:56:15Z

Hi @ywangd, I've created a changelog YAML for you.

elasticsearchmachine · 2023-09-20T02:57:01Z

Pinging @elastic/es-distributed (Team:Distributed)

ywangd · 2023-09-20T02:58:31Z

server/src/main/java/org/elasticsearch/common/blobstore/BlobStore.java

+    @Deprecated(forRemoval = true)
    void deleteBlobsIgnoringIfNotExists(Iterator<String> blobNames) throws IOException;

+    // TODO: Remove the default implementation and require each blob store to implement this method. Once it's done, remove the
+    // the above overload version that does not take the Purpose parameter.
+    /**
+     * Delete all the provided blobs from the blob store. Each blob could belong to a different {@code BlobContainer}
+     * @param blobNames the blobs to be deleted
+     * @param purpose the purpose of the {@code BlobContainer} associated to the blobs to be deleted. It should be set
+     *                to {@code Purpose.GENERIC}, if the blobs are from multiple {@code BlobContainer}s.
+     */
+    default void deleteBlobsIgnoringIfNotExists(Iterator<String> blobNames, Purpose purpose) throws IOException {
+        if (purpose == Purpose.GENERIC) {
+            deleteBlobsIgnoringIfNotExists(blobNames);
+        } else {
+            throw new UnsupportedOperationException();
+        }
+    }


As David suggested, I will perform the cleanup of these two methods in a follow-up and avoid breaking builds on either side.

ywangd · 2023-09-20T03:03:50Z

@elasticmachine update branch

DaveCTurner

I think attaching the Purpose to the path to the container will be ok for our current needs, but I worry that it won't be flexible enough for some related future work: we want to distinguish data from metadata operations when taking a snapshot, and I can see value in being able to distinguish searchable-snapshot-related read operations from regular snapshot reads too.

Have we considered making this a per-operation property rather than a property of the BlobContainer? This will involve changing more call sites here of course but no more real complexity and a great deal more flexibility in future.

server/src/main/java/org/elasticsearch/common/blobstore/BlobPath.java

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

server/src/main/java/org/elasticsearch/common/blobstore/BlobPath.java

henningandersen · 2023-09-20T09:41:25Z

Have we considered making this a per-operation property rather than a property of the BlobContainer? This will involve changing more call sites here of course but no more real complexity and a great deal more flexibility in future.

I agree, I'd like to at least try out that approach too to see how big of a change it is.

ywangd · 2023-09-20T10:03:16Z

Thanks for the feedback. I was under the impression that once a BlobContainer is created, it can be used for only a single purpose since it deals with a flat list of files and no nested sub-folders. But apparently this was an invalid assumption. We can store both data and metadata under the same blobPath, e.g. shard level metadata and data files. I will take another attempt to make the change per operation.

ywangd · 2023-09-20T11:41:27Z

Assuming "per operation" means "per method of the BlobContainer" class, I think it is achievable without having to change signature of every method. We still attach purpose to BlobContainer so that it is easily referenceable in all methods without having to change method signatures. Add a new BlobContainer.withPurpose(Purpose) method that returns a new BlobContainer with the given purpose. At each call site, we call withPurpose first when necessary and subsequent operation methods can remain unchanged. So it is roughly something like the follows:

class BlobContainer {
    Purpose purpose = Purpose.SNAPSHOT;  // default

    BlobContainer withPurpose(Purpose purpose) {
        return new BlobContainer(..., purpose);
    }

    // operation method
    void writeBlob(...) {
        // the method can reference purpose as instance variable
    }

    ...
}

// Sample call site usage
class TranslogFileUploadTask {
    void doRun() {
        blobContainer.withPurpose(Purpose.TRANSLOG)  // configure the purpose per operation
            .writeBlob(...);  // as is today
    }
}

I think the above can achieve the "per operation" variety with likely less cascading code changes. What do you think?

DaveCTurner · 2023-09-20T18:15:48Z

Add a new BlobContainer.withPurpose(Purpose) method that returns a new BlobContainer with the given purpose.

I'd rather not create a new BlobContainer for each method as would happen if we did that. And this approach would effectively make the purpose optional, which isn't what we want. It'd be better in the long run to just add a parameter throughout.

…ethod

ywangd · 2023-09-21T02:42:31Z

server/src/main/java/org/elasticsearch/common/blobstore/BlobContainer.java

+    @Deprecated(forRemoval = true)
+    default InputStream readBlob(String blobName) throws IOException {
+        return readBlob(OperationPurpose.SNAPSHOT, blobName);
+    }


Similarly, methods marked as deprecated in this class will be removed once the necessary changes are made in the other repo. They are here to avoid breaking builds for now.

ywangd · 2023-09-26T12:43:42Z

Thanks for the reviews. I have updated the PR accordingly. The most notable update is that I removed the changes to actual stats collection so that this PR focuses on adding the new purpose parameter (as suggested here). This is a good call since I think we are not entirely aligned on the new stats structure. Deferring it gives us more time for discussion. Note that I still kept the refactoring for getMetricsCollector since it has no functional change and will be generally more useful in follow-ups.

ywangd · 2023-09-28T23:00:03Z

I believe all reviews have been addressed. I'd appreciate if you could take another look. Thanks!

DaveCTurner

I left a couple more (somewhat superficial) comments.

DaveCTurner · 2023-10-02T07:30:50Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

-    final RequestMetricCollector multiPartUploadMetricCollector;
-    final RequestMetricCollector deleteMetricCollector;
-    final RequestMetricCollector abortPartUploadMetricCollector;
+    private final StatsCollectors statsCollectors = new StatsCollectors();


Could we revert these changes to stats collection too? That way this PR is just about adding the purpose parameter to all the APIs, which is noisy but low-risk, and follow-ups which change the behaviour like here will be easier to review and will let us pin down any later problems with git bisect.

Sure I reverted these changes as well. I believe this PR is now a pure refactor. Hence I adjusted the labels accordingly as well.

DaveCTurner · 2023-10-02T07:33:01Z

server/src/main/java/org/elasticsearch/common/blobstore/BlobPurpose.java

+
+package org.elasticsearch.common.blobstore;
+
+public enum BlobPurpose {


Sorry for nitpicking about naming here but I don't like the new name BlobPurpose. We're not distinguishing blobs by purpose, it's the operations on them.

OK I reverted it back to OperationPurpose.

DaveCTurner

LGTM thanks for the extra iterations Yang

henningandersen

LGTM.

Left a few minor comments but no need for another round. Thanks for the extra iterations on this.

henningandersen · 2023-10-02T08:35:51Z

...c/internalClusterTest/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java

+
+        final BlobPath blobPath = repository.basePath().add(randomAlphaOfLength(10));
+        final BlobContainer blobContainer = blobStore.blobContainer(blobPath);
+        final OperationPurpose purpose = randomValueOtherThan(OperationPurpose.SNAPSHOT, () -> randomFrom(OperationPurpose.values()));


I wonder why we exclude SNAPSHOT here? I'd expect the test to also succeed for that and furthermore for the validation of SNAPSHOT to be equally valid?

This test had two parts where the 1st part uses SNAPSHOT and the 2nd part randomizes from purposes other than SNAPSHOT. Since this PR is now a pure refactor, the test mostly lost its purpose. Now it really just shows the refactor has no impact on output. In this case, you are right that there is no need to exclude SNAPSHOT. I have updated accordingly.

henningandersen · 2023-10-02T08:38:36Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

@@ -243,9 +244,14 @@ public void deleteBlobsIgnoringIfNotExists(Iterator<String> blobNames) throws IO
        }
    }

-    private void deletePartition(AmazonS3Reference clientReference, List<String> partition, AtomicReference<Exception> aex) {
+    private void deletePartition(


nit: all other places have purpose as first arg, I'd like to keep it consistent, so can we move it to be first here too?

Thanks for catching it. It was my intention to keep it consistently but apparently missed this one and the one below. Corrected now.

henningandersen · 2023-10-02T08:38:57Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

@@ -264,7 +270,7 @@ private void deletePartition(AmazonS3Reference clientReference, List<String> par
        }
    }

-    private static DeleteObjectsRequest bulkDelete(S3BlobStore blobStore, List<String> blobs) {
+    private static DeleteObjectsRequest bulkDelete(S3BlobStore blobStore, List<String> blobs, OperationPurpose purpose) {


nit: all other places have purpose as first arg, I'd like to keep it consistent, so can we move it to be first here too?

henningandersen · 2023-10-02T08:41:29Z

...pository-url/src/test/java/org/elasticsearch/common/blobstore/url/HttpURLBlobStoreTests.java

-        expectThrows(IOException.class, () -> container.readBlob(blobName, content.length + 1, content.length).read());
+        expectThrows(
+            IllegalArgumentException.class,
+            () -> container.readBlob(OperationPurpose.SNAPSHOT, blobName, -1, content.length).read()


It would be nice to randomize the purpose here (and in many places elsewhere). I think it can be a follow-up though.

Yep we should have follow-ups for actual usages of the purpose parameter. In this PR, all callsite simply uses the SNAPSHOT as the default.

henningandersen · 2023-10-02T08:43:07Z

server/src/main/java/org/elasticsearch/common/blobstore/BlobContainer.java

-     * @param   blobName
-     *          The name of the blob whose existence is to be determined.
-     * @return  {@code true} if a blob exists in the {@link BlobContainer} with the given name, and {@code false} otherwise.
+     * @param purpose The purpose of the operation, useful for stats collection.


I think there are several potential uses of this, stats collection, storage classes, priority. Perhaps we can move the examples to OperationPurpose javadoc and leave the example out here (and in other methods in this class)?

Suggested change

* @param purpose The purpose of the operation, useful for stats collection.

* @param purpose The purpose of the operation

Makes sense. I removed it from individual method and added it (with a bit more explanation) to the Enum class. Btw, the other usages sound pretty exciting!

henningandersen · 2023-10-02T08:45:37Z

server/src/main/java/org/elasticsearch/common/blobstore/OperationPurpose.java

+
+package org.elasticsearch.common.blobstore;
+
+public enum OperationPurpose {


I see us mentioning stats collection in every method javadoc where this is passed. I think that is only one use out of potentially several. Instead, we could supply that information here and leave the method level javadoc without the use case.

…lastic#99615) Today blobstore stats are collected against each HTTP operation, e.g. Get, List. This is not granular enough because the same HTTP operration can be performed for different purposes, e.g. cluster state, indices or translog. This PR adds a new Purpose enum to provide further breakdown for the same HTTP operation. Relates: ES-6800

All their usages have been replaced by corresponding new versions. Relates: elastic#99615

All their usages have been replaced by corresponding new versions. Relates: #99615

A new no-op OperationPurpose parameter is added in elastic#99615 to all blob store/container operation method. This PR updates the s3 stats collection code to actually use this parameter for finer grained stats collection and reports. Stats are reported per combination of operation and operation purpose. A sample output is as the follows: ``` { "ListObjects": 2, "GetObject": 1, "PutObject": 2, "PutMultipartObject": 0, "AbortMultipartObject": 0, "DeleteObjects": 1, "GetObject/ClusterState": 1, "PutObject/ClusterState": 1, "DeleteObjects/Translog": 1, "ListObjects/Indices": 1 } ``` The changes are made with BWC in mind, i.e. existing stats reports with default operation purpose will remain unchanged. For an example, the key "ListObjects" is equivalent "ListObjects/Snapshot". But we omit the default purpose in the stats key so that it is backwards compatible. Relates: elastic#99615 Relates: ES-6800

A new no-op OperationPurpose parameter is added in #99615 to all blob store/container operation method. This PR updates the s3 stats collection code to actually use this parameter for finer grained stats collection and reports. This differentiation between purposes are kept internally for now. The stats are currently aggregated over operations for existing stats reporting. This means responses from both GetRepositoriesMetering API and GetBlobStoreStats API will not be changed. We will have follow-ups to expose the finer stats separately. Relates: #99615 Relates: ES-6800

ywangd added 2 commits September 18, 2023 13:34

wip

0fcebdb

Differentiate between purposes for the same blobstore operation

9bf575e

elasticsearchmachine added the v8.11.0 label Sep 18, 2023

DaveCTurner reviewed Sep 18, 2023

View reviewed changes

ywangd added 4 commits September 18, 2023 17:51

Address feedback to drop nullable

0a39942

address feedback and add deprecation

e983fec

fix test

375f915

tweak and add tests

465f125

ywangd added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Sep 20, 2023

Update docs/changelog/99615.yaml

49b6948

ywangd marked this pull request as ready for review September 20, 2023 02:56

ywangd requested review from DaveCTurner and henningandersen September 20, 2023 02:56

elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Sep 20, 2023

ywangd commented Sep 20, 2023

View reviewed changes

Merge branch 'main' into s3-more-granular-metrics

b3eac9c

DaveCTurner reviewed Sep 20, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/common/blobstore/BlobPath.java Outdated Show resolved Hide resolved

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java Outdated Show resolved Hide resolved

idegtiarenko reviewed Sep 20, 2023

View reviewed changes

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java Outdated Show resolved Hide resolved

idegtiarenko reviewed Sep 20, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/common/blobstore/BlobPath.java Outdated Show resolved Hide resolved

ywangd added 2 commits September 21, 2023 12:36

Address feedback to add the new purpose parameter to each operation m…

787552f

…ethod

tweak

74cbcb2

ywangd commented Sep 21, 2023

View reviewed changes

ywangd added 2 commits September 26, 2023 22:12

rename and remove changes to actual stats collection

27527d7

Merge remote-tracking branch 'origin/main' into s3-more-granular-metrics

ac2cded

ywangd requested review from DaveCTurner and henningandersen September 26, 2023 12:43

ywangd added 2 commits September 27, 2023 01:36

fix method delegation

eb723f9

Merge remote-tracking branch 'origin/main' into s3-more-granular-metrics

dfde25f

DaveCTurner reviewed Oct 2, 2023

View reviewed changes

ywangd added 2 commits October 2, 2023 18:53

Merge remote-tracking branch 'origin/main' into s3-more-granular-metrics

ac94004

more revert to address feedback

e4a96e2

ywangd added >refactoring and removed >enhancement labels Oct 2, 2023

Delete docs/changelog/99615.yaml

cd4e17f

ywangd requested a review from DaveCTurner October 2, 2023 08:17

DaveCTurner approved these changes Oct 2, 2023

View reviewed changes

henningandersen approved these changes Oct 2, 2023

View reviewed changes

address feedback

2968087

ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 2, 2023

fix compilation

03e3a62

elasticsearchmachine merged commit 5628392 into elastic:main Oct 2, 2023

ywangd deleted the s3-more-granular-metrics branch October 2, 2023 10:37

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Oct 3, 2023

Remove deprecated methods from BlobStore and BlobContainer

1cb73dd

All their usages have been replaced by corresponding new versions. Relates: elastic#99615

ywangd mentioned this pull request Oct 3, 2023

Remove deprecated methods from BlobStore and BlobContainer #100177

Merged

ywangd added a commit that referenced this pull request Oct 3, 2023

Remove deprecated methods from BlobStore and BlobContainer (#100177)

1c86da0

All their usages have been replaced by corresponding new versions. Relates: #99615

ywangd mentioned this pull request Oct 3, 2023

Record operation purpose for s3 stats collection #100236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Differentiate stats for the same blobstore operation with purposes #99615

Differentiate stats for the same blobstore operation with purposes #99615

ywangd commented Sep 18, 2023 •

edited

Loading

DaveCTurner Sep 18, 2023

ywangd Sep 18, 2023

DaveCTurner Sep 18, 2023

ywangd Sep 18, 2023

elasticsearchmachine commented Sep 20, 2023

elasticsearchmachine commented Sep 20, 2023

ywangd Sep 20, 2023

ywangd commented Sep 20, 2023

DaveCTurner left a comment

henningandersen commented Sep 20, 2023

ywangd commented Sep 20, 2023

ywangd commented Sep 20, 2023 •

edited

Loading

DaveCTurner commented Sep 20, 2023

ywangd Sep 21, 2023

ywangd commented Sep 26, 2023

ywangd commented Sep 28, 2023

DaveCTurner left a comment

DaveCTurner Oct 2, 2023

ywangd Oct 2, 2023

DaveCTurner Oct 2, 2023

ywangd Oct 2, 2023

DaveCTurner left a comment

henningandersen left a comment

henningandersen Oct 2, 2023

ywangd Oct 2, 2023

henningandersen Oct 2, 2023

ywangd Oct 2, 2023

henningandersen Oct 2, 2023

henningandersen Oct 2, 2023

ywangd Oct 2, 2023

henningandersen Oct 2, 2023

ywangd Oct 2, 2023

henningandersen Oct 2, 2023


		package org.elasticsearch.common.blobstore;

		public enum BlobPurpose {

	* @param purpose The purpose of the operation, useful for stats collection.
	* @param purpose The purpose of the operation


		package org.elasticsearch.common.blobstore;

		public enum OperationPurpose {

Differentiate stats for the same blobstore operation with purposes #99615

Differentiate stats for the same blobstore operation with purposes #99615

Conversation

ywangd commented Sep 18, 2023 • edited Loading

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

elasticsearchmachine commented Sep 20, 2023

elasticsearchmachine commented Sep 20, 2023

Choose a reason for hiding this comment

ywangd commented Sep 20, 2023

DaveCTurner left a comment

Choose a reason for hiding this comment

henningandersen commented Sep 20, 2023

ywangd commented Sep 20, 2023

ywangd commented Sep 20, 2023 • edited Loading

DaveCTurner commented Sep 20, 2023

Choose a reason for hiding this comment

ywangd commented Sep 26, 2023

ywangd commented Sep 28, 2023

DaveCTurner 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

DaveCTurner left a comment

Choose a reason for hiding this comment

henningandersen 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

ywangd commented Sep 18, 2023 •

edited

Loading

ywangd commented Sep 20, 2023 •

edited

Loading