Progress reporter promoted to core and used in Http Client.

[kasobol-msft]

This PR:

• Adds ProgressReporter and ProgressListener to the core (copies exist across SDKs and should get replaced by this over time).
• The progress reporter is a tree that can branch out progress reporting to concurrent processes or logical units (like block uploads).
• Add Contexts utility to manipulate Context and set cross-cutting properties there.
• Implement progress reporting across existing HttpClient (with battery of tests).

Note

1. The goal of this PR is to establish baseline of this functionality, i.e. make it work and develop test converage.
   For some clients (JDK, Vertx) there's room for improvement to push it closer to the socket.write, but it's not a goal of this PR to get there now.
2. The progress reporting itself is standalone functionality that can be used on any flow, not only http client uploads.

Prototypes

1. Storage - WIP - Storage upload from file - binary data #29277
2. Comms - kasobol-msft@9f37bae
3. Comms - kasobol-msft@460f7b2

[azure-sdk]

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-core
azure-core-test

[kasobol-msft]

/azp run java - core - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JonathanGiles]

Should we clarify what determines if something that is user-settable via a Context becomes elevated to be an API on Contexts? For example, I see above code that gets the AZURE_RESPONSE_TIMEOUT and AZURE_EAGERLY_READ_RESPONSE values out of Context. At what stage do we do this elevation, and then, does progress reporter meet that criteria?

[kasobol-msft]

I'd say that stuff that has to cross core-sdks boundary is a candidate where HLC logic has interest in orchestrating it.
Especially when there's N:1 relationship of interest, i.e. N SDKS need to use it with core (core is 1 here).
For example:

• progress reporting is present in 3 storage services plus comms. We should not use magic key for this.
• The AZURE_EAGERLY_READ_RESPONSE is a contract between RestProxy and HttpClients. Doesn't cross core boundary. Shouldn't be API
• AZURE_RESPONSE_TIMEOUT (or some form of timeout tbd). We have sync APIs that take Duration timeout in storage, tables. This needs to (plus-minus math transformations in retry policy) travel across sdks-core boundary. This is a good candidate for API.

[JonathanGiles]

This could have an @FunctionalInterface annotation (at class level). I also wonder if we should be reporting the total size, if known, so that users can more easily determine percentage progress?

[kasobol-msft]

It's replacing https://github.com/Azure/azure-sdk-for-java/search?q=%22interface+ProgressReceiver%22 .

I'll add @FunctionalInterface

I wouldn't add total size eagerly. We have APIs (e.g. in shares) that take Fluxes or InputStreams without defined length and chunk them.
If caller knows the length they can easily implement

class MyReporter {
 private final long totalSize;
 public MyReporter(long totalsize);
 public onProgress(long delta) {
 System.out.println(delta/totalSize + " completed);
}
}

shareClient.upload(flux, new MyReporter(knownSize));

If the caller wouldn't know total size upfront, we wouldn't know it either.

[kasobol-msft]

After conducting this experiment 36b58fd . I think we should:

• match signature of existing ProgressReceiver in storage and comms
• eventually make storage/comms.ProgressReceiver extend core.ProgressReceiver so that users with existing implementations will be able to use it with new apis where we'd use core's type.

[kasobol-msft]

comms prototype for reference as well kasobol-msft@9f37bae

[JonathanGiles]

Do you feel this is necessary in the first instance?

[kasobol-msft]

Yes. We have to be able to track progress for chunked uploads. Including cases where single block hits retries (and partial progress has to be reset).

Example from prototype.
https://github.com/kasobol-msft/azure-sdk-for-java/blob/7c65a6c06ec9845ebf394077ff83e92be0950dfe/sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobAsyncClient.java#L1012

[alzimmermsft]

Is this API being used anywhere yet?

[kasobol-msft]

https://github.com/kasobol-msft/azure-sdk-for-java/blob/7c65a6c06ec9845ebf394077ff83e92be0950dfe/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryPolicy.java#L112-L115

Other option is to make HttpPipelineCallContext.getContext public.

[srnagar]

I don't see any reason not to make getContext() in HttpPipelineCallContext public. We should just do that instead of coupling Contexts with an HTTP pipeline type.

[kasobol-msft]

public HttpPipelineCallContext.getContext() sounds good.

[alzimmermsft]

Are there other commonly used Context keys that should be given APIs here (eventually, not in this PR)

[kasobol-msft]

This guy after figuring out how timeout family looks like

azure-sdk-for-java/sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClient.java

Line 111 in fc24232

long effectiveResponseTimeout = context.getData(AZURE_RESPONSE_TIMEOUT)

.

Also see discussion above #29495 (comment)

[srnagar]

I don't see any reason not to make getContext() in HttpPipelineCallContext public. We should just do that instead of coupling Contexts with an HTTP pipeline type.

[srnagar]

We have used different naming conventions for known context keys. We should standardize the naming of well-known context keys.

• Tracing key
• Read response eagerly

[kasobol-msft]

There's also this one

azure-sdk-for-java/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java

Lines 69 to 72 in 515b782

	/**
	* Key for {@link Context} to pass request retry count metadata for logging.
	*/
	public static final String RETRY_COUNT_CONTEXT = "requestRetryCount";

There's more context keys in Tracer which are kind of all over the place.

If we're to standardize which format do we pick ? I'd go for com.azure.core.my.awesome.key and encourage long non ambiguous keys (like reactor does).

Also, I'd suggest we pick a standard and keep it going forward and log issue to revisit existing keys (no scope creep).

[srnagar]

Yes, I am fine with using key names similar to Java package names.

[srnagar]

Given the name of this method, the type should be ProgressReporter or this method should be receiveProgress to match the type name. Also, if we are thinking of this to be a generic progress tracker (per comment above), should this be bytesTransferred? This could potentially track progress of anything like number of files uploaded or number of pages received in a paged collection.

[kasobol-msft]

Yeah. we can definietly change name of the parameter to something more generic.

When it comes to symbols. I had ProgressListener.onProgress before but in offline discussion with @JonathanGiles we figured that diverging too much from existing pattern is going to cause user pain and noise on existing types like options bags that take progressreceivers today if they have implementations based on existing types.

1. We want to match signature of existing ProgressReceivers like this one https://github.com/kasobol-msft/azure-sdk-for-java/blob/36b58fde61222042d7915b88a92313633aacb819/sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/ProgressReceiver.java#L27 and make them extend new core type https://github.com/kasobol-msft/azure-sdk-for-java/blob/36b58fde61222042d7915b88a92313633aacb819/sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/ProgressReceiver.java#L20 . The reason for that is good deprecation story when they have implementations based on existing interfaces and we stop developing APIs based on old interfaces and deprecate them eventually.
2. As for the type name we could do ProgressReporter and rename the other ProgressReporter (the one that is called by our impl to publish) to ProgressCollector/ProgressPublisher or so.

Or perhaps if we want to revisit naming we could make something like

core.ProgressListener {
 void onProgress(progress);
}

storage.ProgressReceiver {
 void reportProgress(progress);
 default onProgress(progress) {
   //i.e. delegate to old abstraction by default.
   reportProgress(progress);
 }
}

Please share your thoughts.

[srnagar]

Yeah, I think ProgressReporter seems like a more accurate name for what this interface does (reporting progress). For the other type, I like ProgressPublisher but Publisher is overloaded with reactive stream publishers. Other names to consider:

• ProgressTracker
• ProgressCollector (mentioned above)

[kasobol-msft]

@JonathanGiles Please chime in on naming here.

[JonathanGiles]

I think the choices discussed above can boil down to either ProgressReporter/reportProgress, ProgressListener/onProgress, or ProgressReceiver/reportProgress. From the standpoint of a developer who is providing this interface implementation, they are expecting to listen to (or receive) progress, rather than report progress. In this interpretation, I think we should rule out ProgressReporter. That leaves us with ProgressListener or ProgressReceiver. I don't have a strong preference here, although I would lean towards a Listener being more commonly understood by developers as a general term.

In terms of onProgress vs reportProgress, I again think from a consumers point of view, they are not reporting progress, and we should probably name the API from the consuming users standpoint, so onProgress or receiveProgress or handleProgress or consumeProgress or consume or anything along these lines should work fine.

In the absence of a decision, my suggestion would be to use ProgressListener with a handleProgress method.

[kasobol-msft]

I sympathize with @JonathanGiles 's pov on naming. However.

Are we fine with this (and it's copies)

azure-sdk-for-java/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/ProgressReceiver.java

Lines 17 to 25 in 0a1b960

	public interface ProgressReceiver {
	/**
	* The callback function invoked as progress is reported.
	*
	* @param bytesTransferred The total number of bytes transferred during this transaction.
	*/
	void reportProgress(long bytesTransferred);
	}

becoming

package com.azure.storage.common;

// Eventually get rid of this type.
public interface ProgressReceiver extends ProgressListener {

    @Deprecated
    void reportProgress(long bytesTransferred);
    
    default void onProgress(long progress) {
       reportProgress(progress);
    }
}

??

[JonathanGiles]

Yes, I'm fine with that (although it'll be handleProgress rather than onProgress, unless I get overruled).

[kasobol-msft]

If we're going with ProgressListener.handleProgress then I'll keep ProgressReporter.reportProgress as is.

[kasobol-msft]

Sample deprecation kasobol-msft@460f7b2 .

[srnagar]

I am considering if we need this additional type because it's not easily discoverable. Context is visible because it is present in all the withResponse methods of sync client. So, we can move the known key-value setters to Context itself.

[kasobol-msft]

@JonathanGiles please chime in here.

One concern that I'd have with that approach is growing list of getters and setters that over time obscures the Context.
We create contract by defining reserved key values so these wouldn't be classic get/set pairs.

If we're unsure and we anyway have to define public constants (see discussion below) then perhaps we could ride on public constants until we hate it and defer creation of this type.

[JonathanGiles]

I generally don't worry too much about the discoverability of this type. It falls into the category of 'support type' that generally seems common in Java, e.g. Collections, StreamSupport, etc.

[srnagar]

I am fine with this new type. We should update documentation and probably replace Context.NONE in code snippets with Contexts.empty() to guide users toward this type.

[JonathanGiles]

Contexts.empty() returns a Contexts, so it is not directly a drop-in replacement for Context.NONE, although we could introduce a Contexts.none() that is a drop-in replacement (although I'm not sure the distinction between Contexts.empty() and Contexts.none() is clear enough, so I wouldn't do this automatically.

[srnagar]

How does the user use this when working with an async client?

This will need creating Azure context first, then converting to reactor context and then internally will be converted to Azure context again.

Should we make the context key (PROGRESS_REPORTER_CONTEXT_KEY) public, so that users can directly set the key, value in reactor context?

Context azureContext = Contexts.empty().setProgressReporter(reporter).getContext();
reactor.util.context.Context reactorContext = FluxUtil.toReactorContext(azureContext);
asyncClient.foo().contextWrite(reactorContext);

[kasobol-msft]

I did this in the prototype
https://github.com/kasobol-msft/azure-sdk-for-java/blob/7c65a6c06ec9845ebf394077ff83e92be0950dfe/sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobAsyncClient.java#L1010-L1017
Which is waste of allocation as you say.
We should make well known keys public constants. Or perhaps we should also create ReactorContexts that mirrors Contexts.

If we're to define public constants where this would be? We don't have a good place for this, so things like azure-eagerly-read-response are scattered across packages.

[srnagar]

We could define all these public constants in Contexts class itself. The other option is to define them in Context like we do for Configuration.

[srnagar]

Should the key name also include http if the request we are referring to is a HTTP request?

[kasobol-msft]

It's a good idea. We should be specific here to not run into ambiguity problems later. (I'll also apply this to get/set).

[srnagar]

Since we are making this public, we should also update the javadoc to mention that the Context returned is a snapshot of the context at the time this method was called and updates to the context will not be reflected in the instance returned from this method.

[JonathanGiles]

Two things:

1. s/ProgressReporter/ProgressListener
2. Just to be clear - the user would need to getContext() and pass that in, rather than expect to mutate the existing Context passed in to the with method with the additional progressReporter. I sat here for a second thinking "do we have two different usage patterns here?" before concluding that the naive (and incorrect, but only with knowledge about how Context works) interpretation of the code sample above would be to do the following:

Context ctx = Context.NONE;
Contexts.with(ctx).setProgressListener(progressListener);
httpClient.send(request, ctx);

[kasobol-msft]

Changed to sample, see effect here https://github.com/kasobol-msft/azure-sdk-for-java/blob/progress-reporter/sdk/core/azure-core-http-netty/CHANGELOG.md#features-added

[JonathanGiles]

Same as above

[JonathanGiles]

Does this need to be public? If so, I'd be inclined to move this to Contexts as static fields named KEY_* or *_KEY.

[kasobol-msft]

For the first question. This is to make insertions in async flows more memory efficient.
I.e. this

       ProgressReporter progressReporter = null;
        Flux.just(1)
            .contextWrite(
                ctx -> ctx.put(Contexts.Keys.HTTP_REQUEST_PROGRESS_REPORTER, progressReporter)
            ).subscribe();

is less involved than

        ProgressReporter progressReporter = null;
        Flux.just(1)
            .contextWrite(
                FluxUtil.toReactorContext(
                    Contexts.empty().setHttpRequestProgressReporter(progressReporter).getContext()
                )
            ).subscribe();

For the second question.
We could do KEY_* (i.e. prefix is better). However, if I type Contexts. in the IDE I'm going to have all those keys show up in hints. Isn't this a concern?

[kasobol-msft]

@rickle-msft Yeah, this particular piece got attention.

azure-sdk-for-java/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/ProgressReporter.java

Lines 101 to 110 in df98d84

	/*
	It is typically a bad idea to lock around customer code (which the progressReceiver is) because they could
	never release the lock. However, we have decided that it is sufficiently difficult for them to make their
	progressReporting code threadsafe that we will take that burden and the ensuing risks. Although it is the
	case that only one thread is allowed to be in onNext at once, there are multiple independent
	requests happening at once to stage/download separate chunks, so we still need to lock either way.
	*/
	transferLock.lock();
	this.progressReceiver.reportProgress(this.totalProgress.addAndGet(bytesTransferred));
	transferLock.unlock();

The comment there seems to promise that by introducing the lock magically makes user code thread safe and pushes that burden on us... Which is overpromising...

That lock in existing reporters has more traps than benefits, i.e.:

1. with or without lock there's no guarantee of sequencing in paralleled scenarios.
2. we're not assuring any thread safety, customer has to use volatile fields in their handler.
3. if we happen to be on a thread where reactor.core.scheduler.Schedulers.isInNonBlockingThread() (or on NIO single threaded event loop) we might run into deadlock/resource starvation.

Therefore we'd rather guide user towards success, by

1. letting them know that they have to provide thread safe implementation. Here https://github.com/kasobol-msft/azure-sdk-for-java/blob/67837d151e5185897506627de50f5dbefa84abaf/sdk/core/azure-core/src/main/java/com/azure/core/util/ProgressListener.java#L59-L62
2. not locking by default, if user falls into traps above they can mitigate by altering their code.