Expose operation and request counts separately in repository stats #117530

nicktindall · 2024-11-26T05:53:18Z

We discovered that the "request counts" for blob store operations were inconsistently reported between Azure and S3 implementations. S3 counted the requests (including retries) and Azure counted operations (not including retries).

This PR changes the BlobStore implementations to report separate operations and requests counts, so we can use operations counts for places where we're interested in reporting costs (e.g. the metering endpoint, also changed in this PR) and we can use request counts for reporting behaviour/performance (e.g. benchmarking, that might be interested in an increase in throttled requests).

The stats for each BlobStore operation will represented by an EndpointStats object which includes two values:

requests, the request count
operations, the operation count

Relates ES-9767
Fixes #104443

…ts endpoints

…tStats

…ng_stats_endpoints # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

elasticsearchmachine · 2024-11-28T03:21:16Z

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

elasticsearchmachine · 2024-11-28T03:32:25Z

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

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

…ng_stats_endpoints # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

ywangd · 2024-11-28T06:53:44Z

It's not immediately obvious to me. Does this PR change any user visible behaviour, i.e. GetRepositoriesMetering API? If the impact is only to the internal API, we should not label it as >bug but instead >non-issue.

nicktindall · 2024-11-28T07:59:39Z

It's not immediately obvious to me. Does this PR change any user visible behaviour, i.e. GetRepositoriesMetering API? If the impact is only to the internal API, we should not label it as >bug but instead >non-issue.

It does, because now S3 request_count will be operations not requests, but the difference between those counts is miniscule. So I would say effectively there is no visible change.

…ng_stats_endpoints

ywangd

Looking great. I only have some minor comments.

ywangd · 2024-12-02T03:48:55Z

modules/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureBlobStore.java

+ EndpointStats addTo(EndpointStats other) {
+ long ops = operations.sum() + other.operations();
+ long reqs = requests.sum() + other.requests();
+ return new EndpointStats(ops, reqs);
+ }


Nit: Why do we need this method given we already have EndpointStats#add which also uses Math.addExact for overflow detection?

Ah it was just slightly lighter-weight, so rather than doing

return new EndpointStats(operations.sum(), requests.sum()).add(other);

which would create two EndpointStats just to do the addition (seemed excessive to just add some longs), I figured this would just create a single new one with the new values. It may be premature optimisation. At very least I should be doing addExact here.

Removed in 9824022

ywangd · 2024-12-02T03:55:34Z

...-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageOperationsStats.java

+ Map<String, EndpointStats> toMap() {
+ final Map<String, EndpointStats> results = new HashMap<>();
+ final long getOperations = getCount.get();
+ results.put("GetObject", new EndpointStats(getOperations, getOperations));
+ final long listOperations = listCount.get();
+ results.put("ListObjects", new EndpointStats(listOperations, listOperations));
+ final long insertOperations = postCount.get() + putCount.get();
+ results.put("InsertObject", new EndpointStats(insertOperations, insertOperations));


I assume this is temporary and needs to be fixed later to differentiate between requests and operations? If so, might worth to raise a ticket and add a TODO with the ticket number here.

Yep, will do. You're right it's just to meet the requirements of the interface, at the moment GCS doesn't track those numbers separately so we fudge it by returning the single value we do have.

Added in 8143b65

ywangd · 2024-12-02T04:06:41Z

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

+ * @param operations The number of calls
+ * @param requests The number of calls (including retries)
+ */
+public record EndpointStats(long operations, long requests) implements Writeable, ToXContentObject {


How about ActionStats? We have similar classes such as TransportActionStats. Maybe this could be BlobStoreActionStats if we don't mind verbosity. Endpoint feels a bit too generic and it does not align with the fact that we categarize based on purpose as well (in addition to operation/url).

ywangd · 2024-12-02T04:08:16Z

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

+ }
+
+ public EndpointStats {
+ assert operations >= 0 && requests >= 0;


I think we can also assert requests >= operations.

Added in 50a9551

ywangd · 2024-12-02T04:11:18Z

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

+ out.writeLong(operations);
+ out.writeLong(requests);


Since these are always non-negative, we can use writeVLong to save a few bytes

Suggested change

out.writeLong(operations);

out.writeLong(requests);

out.writeVLong(operations);

out.writeVLong(requests);

Similar change will be needed on the read side.

Addressed in 0234b14

ywangd · 2024-12-02T04:12:09Z

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

+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof EndpointStats other) {
+ return requests == other.requests && operations == other.operations;
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(operations, requests);
+ }


Do we need these given it is a record?

Good pickup, no.

Addressed in 0143979

ywangd · 2024-12-02T04:22:52Z

server/src/main/java/org/elasticsearch/repositories/RepositoryStats.java

 public static final RepositoryStats EMPTY_STATS = new RepositoryStats(Collections.emptyMap());

- public final Map<String, Long> requestCounts;
+ public final Map<String, EndpointStats> requestCounts;


Can we rename this field accordingly? It gets a bit confusing reading through other parts of the code.

Addressed in 30968f5

ywangd · 2024-12-02T04:26:33Z

...y-azure/src/test/java/org/elasticsearch/repositories/azure/AzureBlobContainerStatsTests.java

+ assertEquals(statsMapString, 1L, stats.get(statsKey(purpose, AzureBlobStore.Operation.PUT_BLOB)).operations());
+ assertEquals(statsMapString, 1L, stats.get(statsKey(purpose, AzureBlobStore.Operation.LIST_BLOBS)).operations());
+ assertEquals(statsMapString, 1L, stats.get(statsKey(purpose, AzureBlobStore.Operation.GET_BLOB_PROPERTIES)).operations());
+ assertEquals(statsMapString, 1L, stats.get(statsKey(purpose, AzureBlobStore.Operation.PUT_BLOCK)).operations());
+ assertEquals(statsMapString, 1L, stats.get(statsKey(purpose, AzureBlobStore.Operation.PUT_BLOCK_LIST)).operations());
+ assertEquals(statsMapString, 1L, stats.get(statsKey(purpose, AzureBlobStore.Operation.BLOB_BATCH)).operations());


Do we need a test for .requests() as well to maybe show how they behave when there are retries? Or is it already covered somewhere else?

It's kind-of implicitly tested elsewhere because we use requests() to compare against the counts maintained by org.elasticsearch.repositories.azure.AzureBlobStoreRepositoryTests.AzureHTTPStatsCollectorHandler, but I added an explicit test in 90e7061

ywangd · 2024-12-02T04:29:15Z

It does, because now S3 request_count will be operations not requests, but the difference between those counts is miniscule. So I would say effectively there is no visible change.

Thanks for explanation. Since there are technically visible changes but trivial enough, maybe label it as >enhancement is a better choice and we can say the request counting accuracy is improved in the release notes.

elasticsearchmachine · 2024-12-02T04:46:06Z

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

…ng_stats_endpoints

ywangd

LGTM

ywangd · 2024-12-03T05:30:08Z

...g/elasticsearch/xpack/repositories/metering/AbstractRepositoriesMeteringAPIRestTestCase.java

 private void assertRequestCountersAccountedForReads(RepositoryStatsSnapshot statsSnapshot) {
 RepositoryStats repositoryStats = statsSnapshot.getRepositoryStats();
- Map<String, Long> requestCounts = repositoryStats.requestCounts;
+ Map<String, BlobStoreActionStats> requestCounts = repositoryStats.actionStats;


Nit: can we also rename the requestCounts here and other places in this test class to something like actionStats?

Addressed in ca36b82

Track requests and operations separately to better serve metering/sta…

28811b6

…ts endpoints

elasticsearchmachine added the v9.0.0 label Nov 26, 2024

nicktindall added 8 commits November 26, 2024 16:57

Naming

b7af443

add #isZero()

28a4b80

Add test

eaa155f

Fix test

6dee284

Fix test

620aa50

Use request count in ESMockAPIBasedRepositoryIntegTestCase#testReques…

253ba48

…tStats

Merge remote-tracking branch 'origin/main' into ES-9767_update_meteri…

683cf84

…ng_stats_endpoints # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

EndpointStats implements ToXContentObject

7987db3

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Nov 27, 2024

nicktindall added >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Nov 28, 2024

nicktindall changed the title ~~Track requests and operations separately to better serve metering/stats endpoints~~ Track/report requests and operations separately to better serve metering/stats endpoints Nov 28, 2024

Update docs/changelog/117530.yaml

21fa988

nicktindall requested a review from ywangd November 28, 2024 03:21

nicktindall marked this pull request as ready for review November 28, 2024 03:32

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Nov 28, 2024

nicktindall commented Nov 28, 2024

View reviewed changes

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

nicktindall added 2 commits November 28, 2024 15:24

Merge remote-tracking branch 'origin/main' into ES-9767_update_meteri…

ee50d59

…ng_stats_endpoints # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

Fix test

84712c3

nicktindall added >non-issue and removed >bug labels Nov 28, 2024

nicktindall and others added 3 commits November 28, 2024 19:36

Delete docs/changelog/117530.yaml

65f5cb9

Merge branch 'main' into ES-9767_update_metering_stats_endpoints

6320511

Remove legacyValue

e4111fa

Merge remote-tracking branch 'origin/main' into ES-9767_update_meteri…

63b88a3

…ng_stats_endpoints

ywangd reviewed Dec 2, 2024

View reviewed changes

nicktindall added >enhancement and removed >non-issue labels Dec 2, 2024

nicktindall and others added 18 commits December 2, 2024 15:46

Update docs/changelog/117530.yaml

dc9d75d

EndpointStats -> BlobStoreActionStats

d9074e1

Use addExact in StatsCounter#addTo

4714328

De-dupe adding logic

9824022

Remove redundant equals and hashcode

0143979

Additionally constrain requests >= operations

50a9551

Make ZERO final

c7b481c

Use VLong for byte serialization

0234b14

RepositoryStats#requestCounts -> RepositoryStats#actionStats

30968f5

Test that requests and operations are tracked separately

90e7061

Add TODO for GCP implementation

8143b65

Merge remote-tracking branch 'origin/main' into ES-9767_update_meteri…

7a7a589

…ng_stats_endpoints

Descriptive assertion errors

bf09493

Stop asserting requests >= operations

ca4987e

Remove redundant method

f328a86

Merge branch 'main' into ES-9767_update_metering_stats_endpoints

e59f387

Add link to issue to fix S3 4xx errors

d4bcbd5

Merge remote-tracking branch 'origin/main' into ES-9767_update_meteri…

e586f5e

…ng_stats_endpoints

ywangd approved these changes Dec 3, 2024

View reviewed changes

nicktindall added 2 commits December 3, 2024 16:52

Rename variables requestCounts -> actionStats

ca36b82

Tidy up

70b91ba

nicktindall merged commit 7d43d8a into elastic:main Dec 3, 2024
16 checks passed

ywangd mentioned this pull request Jan 13, 2025

[CI] S3BlobStoreRepositoryTests testRequestStats failing #88841

Closed

nicktindall deleted the ES-9767_update_metering_stats_endpoints branch September 10, 2025 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Expose operation and request counts separately in repository stats #117530

Expose operation and request counts separately in repository stats #117530

Uh oh!

nicktindall commented Nov 26, 2024 •

edited

Loading

elasticsearchmachine commented Nov 28, 2024

elasticsearchmachine commented Nov 28, 2024

Uh oh!

ywangd commented Nov 28, 2024

nicktindall commented Nov 28, 2024 •

edited

Loading

ywangd left a comment

ywangd Dec 2, 2024

nicktindall Dec 2, 2024

nicktindall Dec 2, 2024

ywangd Dec 2, 2024

nicktindall Dec 2, 2024

nicktindall Dec 2, 2024

ywangd Dec 2, 2024

ywangd Dec 2, 2024

nicktindall Dec 2, 2024

ywangd Dec 2, 2024

nicktindall Dec 2, 2024

ywangd Dec 2, 2024

nicktindall Dec 2, 2024

nicktindall Dec 2, 2024

ywangd Dec 2, 2024

nicktindall Dec 2, 2024

ywangd Dec 2, 2024

nicktindall Dec 2, 2024

ywangd commented Dec 2, 2024

elasticsearchmachine commented Dec 2, 2024

ywangd left a comment

ywangd Dec 3, 2024

nicktindall Dec 3, 2024

Uh oh!

Labels

3 participants

Expose operation and request counts separately in repository stats #117530

Expose operation and request counts separately in repository stats #117530

Uh oh!

Conversation

nicktindall commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Nov 28, 2024

elasticsearchmachine commented Nov 28, 2024

Uh oh!

ywangd commented Nov 28, 2024

nicktindall commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ywangd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Dec 2, 2024

elasticsearchmachine commented Dec 2, 2024

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Labels

3 participants

nicktindall commented Nov 26, 2024 •

edited

Loading

nicktindall commented Nov 28, 2024 •

edited

Loading