Skip to content

Conversation

@fcofdez
Copy link
Contributor

@fcofdez fcofdez commented May 19, 2020

Add tracking for regular and block uploads on AzureBlobStore

@fcofdez fcofdez added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.9.0 labels May 19, 2020
@fcofdez fcofdez self-assigned this May 19, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

Copy link
Contributor

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor point on the accuracy of the matching, looks good in general. Thanks!

String queryParams = httpURLConnection.getURL().getQuery();
// https://docs.microsoft.com/en-us/rest/api/storageservices/put-block
// https://docs.microsoft.com/en-us/rest/api/storageservices/put-block-list
if (queryParams != null && (queryParams.contains("comp=block") || queryParams.contains("comp=blocklist"))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queryParams.contains("comp=blocklist") is redundant isn't it?
Might be safer to require a blockid= as well if we just see a comp=block just to make sure we're in line with expectations on the API requests we'll see here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I've just added an additional condition to ensure that we're really matching against a PutBlock request. Thanks!

// https://docs.microsoft.com/en-us/rest/api/storageservices/put-block
// https://docs.microsoft.com/en-us/rest/api/storageservices/put-block-list
private boolean isBlockUpload(String request) {
return Regex.simpleMatch("PUT /*/*?*comp=blocklist*", request)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sam as in the production code, matching on comp=blocklist seems redundant when matching comp=block?

return;
}

stats.getOperations.incrementAndGet();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semi-related to this PR since you did it in the new collector, let's add assertions in spots like this one to make sure, we're actually seeing a GET here :) Can probably do the same for the list collector also.

Copy link
Contributor

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Francisco!

@fcofdez fcofdez merged commit 007ab1b into elastic:master May 25, 2020
@fcofdez fcofdez deleted the azure-put-api-calls-tracking branch May 25, 2020 10:53
fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request May 25, 2020
fcofdez added a commit that referenced this pull request May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.9.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants