feat: add batch throttled time to metrics #1413
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| /* | ||
| * Copyright 2021 Google LLC | ||
| * | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions are | ||
| * met: | ||
| * | ||
| * * Redistributions of source code must retain the above copyright | ||
| * notice, this list of conditions and the following disclaimer. | ||
| * * Redistributions in binary form must reproduce the above | ||
| * copyright notice, this list of conditions and the following disclaimer | ||
| * in the documentation and/or other materials provided with the | ||
| * distribution. | ||
| * * Neither the name of Google LLC nor the names of its | ||
| * contributors may be used to endorse or promote products derived from | ||
| * this software without specific prior written permission. | ||
| * | ||
| * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS | ||
| * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT | ||
| * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR | ||
| * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT | ||
| * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, | ||
| * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT | ||
| * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, | ||
| * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY | ||
| * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT | ||
| * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
| * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
| package com.google.api.gax.batching; | ||
|
|
||
| import com.google.api.core.InternalApi; | ||
| import com.google.auto.value.AutoValue; | ||
|
|
||
| /** | ||
| * BatchedCallContext encapsulates context data in a batch call. | ||
| * | ||
| * <p>For internal use only. | ||
| */ | ||
| @InternalApi | ||
| @AutoValue | ||
| public abstract class BatchedCallContext { | ||
|
|
||
| /** Gets element count of the current batch. */ | ||
| public abstract long getElementCount(); | ||
|
|
||
| /** Gets byte count of the current batch. */ | ||
| public abstract long getByteCount(); | ||
|
|
||
| /** Gets total throttled time of the current batch. */ | ||
| public abstract long getTotalThrottledTimeMs(); | ||
|
|
||
| @AutoValue.Builder | ||
| public abstract static class Builder { | ||
|
|
||
| public static Builder newBuilder() { | ||
| return new AutoValue_BatchedCallContext.Builder(); | ||
| } | ||
|
|
||
| /** Gets element count of the current batch. */ | ||
| public abstract Builder setElementCount(long elementCount); | ||
|
|
||
| /** Gets byte count of the current batch. */ | ||
| public abstract Builder setByteCount(long byteCount); | ||
|
|
||
| /** Gets total throttled time of the current batch. */ | ||
| public abstract Builder setTotalThrottledTimeMs(long throttledTimeMs); | ||
|
|
||
| public abstract BatchedCallContext build(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| /* | ||
| * Copyright 2021 Google LLC | ||
| * | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions are | ||
| * met: | ||
| * | ||
| * * Redistributions of source code must retain the above copyright | ||
| * notice, this list of conditions and the following disclaimer. | ||
| * * Redistributions in binary form must reproduce the above | ||
| * copyright notice, this list of conditions and the following disclaimer | ||
| * in the documentation and/or other materials provided with the | ||
| * distribution. | ||
| * * Neither the name of Google LLC nor the names of its | ||
| * contributors may be used to endorse or promote products derived from | ||
| * this software without specific prior written permission. | ||
| * | ||
| * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS | ||
| * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT | ||
| * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR | ||
| * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT | ||
| * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, | ||
| * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT | ||
| * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, | ||
| * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY | ||
| * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT | ||
| * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
| * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
| package com.google.api.gax.tracing; | ||
|
|
||
| import com.google.api.core.ApiFuture; | ||
| import com.google.api.core.InternalApi; | ||
| import com.google.api.gax.batching.BatchedCallContext; | ||
| import com.google.api.gax.rpc.ApiCallContext; | ||
| import com.google.api.gax.rpc.UnaryCallable; | ||
|
|
||
| /** | ||
| * A {@link UnaryCallable} which sends batched requests with context of the current batch. | ||
| * | ||
| * <p>This is public only for technical reasons. | ||
| */ | ||
| @InternalApi | ||
| public abstract class BatchedContextCallable<RequestT, ResponseT> | ||
|
Contributor
There was a problem hiding this comment. Just wondering, can we make some of those InternalApi classes package-private?
Contributor
There was a problem hiding this comment. this class crosses the boundary between tracing and batching so its shared by 2 packages |
||
| extends UnaryCallable<RequestT, ResponseT> { | ||
|
|
||
| /** | ||
| * Performs a call asynchronously with context data of the current batch. | ||
| * | ||
| * @param batchedCallContext {@link BatchedCallContext} to make the call with | ||
| */ | ||
| public abstract ApiFuture<ResponseT> futureCall( | ||
| RequestT request, BatchedCallContext batchedCallContext); | ||
|
|
||
| @Override | ||
| public UnaryCallable<RequestT, ResponseT> withDefaultCallContext( | ||
| final ApiCallContext defaultCallContext) { | ||
| throw new UnsupportedOperationException("withDefaultCallContext() not implemented"); | ||
|
Contributor
There was a problem hiding this comment. Since callables are wrapped in other callables, forming a long "chain of responsibility" pattern it is basically very hard to predic tin which context this method will be called. I.e. if it throws UnsupportedOperatoinException, it is likely that it will actualy get thrown in some valid execution workflow. Can we actually implement this method?
Contributor
There was a problem hiding this comment. Unfortunately, no. This is here to make sure that this class is not misused. We can't implement it generically because the Batcher wont be able to pass it the context |
||
| } | ||
| } | ||
igorbernstein2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| /* | ||
| * Copyright 2021 Google LLC | ||
| * | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions are | ||
| * met: | ||
| * | ||
| * * Redistributions of source code must retain the above copyright | ||
| * notice, this list of conditions and the following disclaimer. | ||
| * * Redistributions in binary form must reproduce the above | ||
| * copyright notice, this list of conditions and the following disclaimer | ||
| * in the documentation and/or other materials provided with the | ||
| * distribution. | ||
| * * Neither the name of Google LLC nor the names of its | ||
| * contributors may be used to endorse or promote products derived from | ||
| * this software without specific prior written permission. | ||
| * | ||
| * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS | ||
| * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT | ||
| * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR | ||
| * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT | ||
| * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, | ||
| * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT | ||
| * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, | ||
| * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY | ||
| * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT | ||
| * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
| * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
| package com.google.api.gax.tracing; | ||
|
|
||
| import com.google.api.core.ApiFuture; | ||
| import com.google.api.core.ApiFutures; | ||
| import com.google.api.core.InternalApi; | ||
| import com.google.api.gax.batching.BatchedCallContext; | ||
| import com.google.api.gax.rpc.ApiCallContext; | ||
| import com.google.api.gax.rpc.UnaryCallable; | ||
| import com.google.api.gax.tracing.ApiTracerFactory.OperationType; | ||
| import com.google.common.base.Preconditions; | ||
| import com.google.common.util.concurrent.MoreExecutors; | ||
|
|
||
| /** | ||
| * This callable wraps a batching callable chain in an {@link ApiTracer} and annotates {@link | ||
| * BatchedCallContext} batching context data. | ||
| * | ||
| * <p>For internal use only. | ||
| */ | ||
| @InternalApi("For internal use by google-cloud-java clients only") | ||
|
Contributor
There was a problem hiding this comment. who else would even use GAX? I would prefer to reserve internal api annotations for things internal to Gax.
Contributor
There was a problem hiding this comment. a dataflow adapter would use this information to publish a counter to let the dataflow service know that the job is being throttled |
||
| public class TracedBatchedContextCallable<RequestT, ResponseT> | ||
| extends UnaryCallable<RequestT, ResponseT> { | ||
|
|
||
| private final ApiTracerFactory tracerFactory; | ||
| private ApiCallContext baseCallContext; | ||
| private final SpanName spanName; | ||
| private final UnaryCallable<RequestT, ResponseT> innerCallable; | ||
|
|
||
| public TracedBatchedContextCallable( | ||
| UnaryCallable<RequestT, ResponseT> innerCallable, | ||
| ApiCallContext callContext, | ||
| ApiTracerFactory tracerFactory, | ||
| SpanName spanName) { | ||
| this.baseCallContext = Preconditions.checkNotNull(callContext); | ||
| this.tracerFactory = Preconditions.checkNotNull(tracerFactory); | ||
| this.spanName = Preconditions.checkNotNull(spanName); | ||
| this.innerCallable = Preconditions.checkNotNull(innerCallable); | ||
| } | ||
|
|
||
| /** | ||
| * Creates an {@link ApiTracer} and annotates batching context data. Performs a call | ||
| * asynchronously. | ||
| */ | ||
| public ApiFuture<ResponseT> futureCall(RequestT request, BatchedCallContext batchedCallContext) { | ||
|
Contributor
There was a problem hiding this comment. Can batchedCallContext just become a part of ApiCallContext? So we will not have to have two methods here, one of which (this one) does not even fit in the overall callables hierarchy (the idea is that callables have their implementation of futureCall(request, apiCallContext), but don't have their own special contexts.
Contributor
There was a problem hiding this comment. Moving this to ApiCallContext is an option. There are 2 approaches:
Localizing the interaction between a batcher and the next callable seems to be the least of the 3 evils
Contributor
There was a problem hiding this comment. Having another futureCall method for a callable to actually do the sole thing why this callable exists does not really make it fit in the chain of responsibility pattern. Instead of seamlessly doing its custom thing, the callable must be called explicitly using the other method (the one with BatchedCallContext), which is not present in other callables in the same chain. This is basically the exact reason, why there is the In other words, the current implementation does make the job done, but it does not use the existing architecture in its idiomatic way. Since using callable must be done via calling Or think about it the other way: if somebody decides to add another wrapper on top of In other words, if you know how to make |
||
| return futureCall( | ||
| request, | ||
| baseCallContext, | ||
| batchedCallContext.getTotalThrottledTimeMs(), | ||
| batchedCallContext.getElementCount(), | ||
| batchedCallContext.getByteCount()); | ||
| } | ||
|
|
||
| /** Calls the wrapped {@link UnaryCallable} within the context of a new trace. */ | ||
| @Override | ||
| public ApiFuture futureCall(RequestT request, ApiCallContext context) { | ||
| ApiCallContext mergedContext = baseCallContext.merge(context); | ||
|
Contributor
There was a problem hiding this comment. Looks like there is a lot of duplication between these two futureCall methods. Can you please rewrite them such that the code is reused, instead of being duplicated?
Contributor
Author
There was a problem hiding this comment. Refactored the methods. |
||
|
|
||
| return futureCall(request, mergedContext, null, null, null); | ||
| } | ||
|
|
||
| private ApiFuture futureCall( | ||
| RequestT request, | ||
| ApiCallContext callContext, | ||
| Long throttledTimeMs, | ||
| Long elementCount, | ||
| Long byteCount) { | ||
| ApiTracer tracer = | ||
| tracerFactory.newTracer(callContext.getTracer(), spanName, OperationType.Batching); | ||
| TraceFinisher<ResponseT> finisher = new TraceFinisher<>(tracer); | ||
|
|
||
| try { | ||
| if (throttledTimeMs != null) { | ||
| tracer.batchRequestThrottled(throttledTimeMs); | ||
| } | ||
| if (elementCount != null && byteCount != null) { | ||
| tracer.batchRequestSent(elementCount, byteCount); | ||
| } | ||
| callContext = callContext.withTracer(tracer); | ||
| ApiFuture<ResponseT> future = innerCallable.futureCall(request, callContext); | ||
| ApiFutures.addCallback(future, finisher, MoreExecutors.directExecutor()); | ||
| return future; | ||
| } catch (RuntimeException e) { | ||
| finisher.onFailure(e); | ||
| throw e; | ||
| } | ||
| } | ||
|
|
||
| public UnaryCallable<RequestT, ResponseT> withDefaultCallContext( | ||
| final ApiCallContext defaultCallContext) { | ||
| return new TracedBatchedContextCallable<>( | ||
| innerCallable, baseCallContext.merge(defaultCallContext), tracerFactory, spanName); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this instanceof? Instanceofs are areally not something we would want to use unless absolutely necessary, since it is usually an indication of mistakes in OOP design of the application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a workaround.
The general idea here is that we want to export state out a batcher so that we can send it to opencensus and eventually to dataflow counters. We dont want to couple the implementation of the tracing to the batcher. So we want to pass the state as a call context. The only 2 approaches I can think of are:
The second approach ended up creating a lot less breakage and least amount of overhead
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the other comment about
ApiCallContextvsBatcherCcallContext, but overall, the more I understand about this change the more I lean towards merging BatcherCallContext thing into ApiCallContext (it still may remain as an aggregated context ojbect with this exact BatcherCallContext name if you want)Can you please clarify what is the extra overhead of the second approach over the first one?
Main issue with the first approach (currently implemented in this PR) is that it breaks chain of responsibility pattern, and if someone adds another wrapper on top of
TracedBatchedContextCallable(which should be completely safe and typical thing to do in the chain of responsibility paradigm) it will break this code, because it makes an assumption thatTracedBatchedCallContextCallableis always the root callable supplied to this BatcherImpl class (which it may be now, but somebody in the future may add some sort ofAwesomeTracedBatchedCallContextCallablewrapping theTracedBatchedCallContextCallable).