Skip to content

Commit b4d2591

Browse files
sydney-munrosydney-munro
committed
pr comments
1 parent 3543adf commit b4d2591

5 files changed

Lines changed: 35 additions & 40 deletions

File tree

‎google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -201,8 +201,7 @@ public void close() throws Exception {
201201

202202
@Override
203203
public Bucket create(BucketInfo bucketInfo, BucketTargetOption... options) {
204-
OpenTelemetryTraceUtil.Span otelSpan =
205-
openTelemetryTraceUtil.startSpan("create(Bucket, BucketTargetOption");
204+
OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("create");
206205
Opts<BucketTargetOpt> opts = Opts.unwrap(options).resolveFrom(bucketInfo).prepend(defaultOpts);
207206
GrpcCallContext grpcCallContext =
208207
opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault());
@@ -249,8 +248,7 @@ public Blob create(
249248
BlobInfo blobInfo, byte[] content, int offset, int length, BlobTargetOption... options) {
250249
Opts<ObjectTargetOpt> opts = Opts.unwrap(options).resolveFrom(blobInfo);
251250
// Start the otel span to retain information of the origin of the request
252-
OpenTelemetryTraceUtil.Span otelSpan =
253-
openTelemetryTraceUtil.startSpan("create(BlobInfo, BlobTargetOption");
251+
OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("create");
254252
try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()) {
255253
return internalDirectUpload(
256254
blobInfo,
@@ -269,7 +267,8 @@ public Blob create(
269267

270268
@Override
271269
public Blob create(BlobInfo blobInfo, InputStream content, BlobWriteOption... options) {
272-
try {
270+
OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("create");
271+
try (OpenTelemetryTraceUtil.Scope ununsed = otelSpan.makeCurrent()) {
273272
requireNonNull(blobInfo, "blobInfo must be non null");
274273
InputStream inputStreamParam = firstNonNull(content, new ByteArrayInputStream(ZERO_BYTES));
275274

@@ -296,7 +295,11 @@ public Blob create(BlobInfo blobInfo, InputStream content, BlobWriteOption... op
296295
ApiFuture<WriteObjectResponse> responseApiFuture = session.getResult();
297296
return this.getBlob(responseApiFuture);
298297
} catch (IOException | ApiException e) {
298+
otelSpan.recordException(e);
299+
otelSpan.setStatus(io.opentelemetry.api.trace.StatusCode.ERROR, e.getClass().getSimpleName());
299300
throw StorageException.coalesce(e);
301+
} finally {
302+
otelSpan.end();
300303
}
301304
}
302305

@@ -812,6 +815,12 @@ public GrpcBlobWriteChannel writer(BlobInfo blobInfo, BlobWriteOption... options
812815
hasher);
813816
}
814817

818+
@Override
819+
public BlobInfo internalDirectUpload(
820+
BlobInfo blobInfo, Opts<ObjectTargetOpt> opts, ByteBuffer buf) {
821+
return internalDirectUpload(blobInfo, opts, buf, null);
822+
}
823+
815824
@Override
816825
public BlobInfo internalDirectUpload(
817826
BlobInfo blobInfo,

‎google-cloud-storage/src/main/java/com/google/cloud/storage/ParallelCompositeUploadWritableByteChannel.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -219,8 +219,8 @@ public void close() throws IOException {
219219
// We never created any parts
220220
// create an empty object
221221
try {
222-
BlobInfo blobInfo =
223-
storage.internalDirectUpload(ultimateObject, opts, Buffers.allocate(0), null);
222+
// TODO: Add in Otel context when available
223+
BlobInfo blobInfo = storage.internalDirectUpload(ultimateObject, opts, Buffers.allocate(0));
224224
finalObject.set(blobInfo);
225225
return;
226226
} catch (StorageException se) {
@@ -287,7 +287,7 @@ private void internalFlush(ByteBuffer buf) {
287287
info -> {
288288
try {
289289
// TODO: Add in Otel context when available
290-
return storage.internalDirectUpload(info, partOpts, buf, null);
290+
return storage.internalDirectUpload(info, partOpts, buf);
291291
} catch (StorageException e) {
292292
// a precondition failure usually means the part was created, but we didn't get the
293293
// response. And when we tried to retry the object already exists.

‎google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ default BlobInfo internalCreateFrom(Path path, BlobInfo info, Opts<ObjectTargetO
3232
throw new UnsupportedOperationException("not implemented");
3333
}
3434

35+
default BlobInfo internalDirectUpload(
36+
BlobInfo blobInfo, Opts<ObjectTargetOpt> opts, ByteBuffer buf) {
37+
throw new UnsupportedOperationException("not implemented");
38+
}
39+
3540
default BlobInfo internalDirectUpload(
3641
BlobInfo info,
3742
Opts<ObjectTargetOpt> opts,

‎google-cloud-storage/src/test/java/com/google/cloud/storage/ITGrpcOpenTelemetryTest.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,16 +88,16 @@ public void runCreateBlob() {
8888
storage.create(BlobInfo.newBuilder(toCreate).build(), content);
8989
TestExporter testExported = (TestExporter) exporter;
9090
List<SpanData> spanData = testExported.getExportedSpans();
91-
// (1) Span when calling create
92-
// (2) Span when passing call to internalDirectUpload
93-
Assert.assertEquals(2, spanData.size());
9491
for (SpanData span : spanData) {
9592
Assert.assertEquals("Storage", getAttributeValue(span, "gcp.client.service"));
9693
Assert.assertEquals("googleapis/java-storage", getAttributeValue(span, "gcp.client.repo"));
9794
Assert.assertEquals(
9895
"com.google.cloud.google-cloud-storage", getAttributeValue(span, "gcp.client.artifact"));
9996
Assert.assertEquals("grpc", getAttributeValue(span, "rpc.system"));
10097
}
98+
Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("create")));
99+
Assert.assertTrue(
100+
spanData.stream().anyMatch(x -> x.getName().contains("internalDirectUpload")));
101101
Assert.assertEquals(spanData.get(1).getSpanContext(), spanData.get(0).getParentSpanContext());
102102
}
103103

‎google-cloud-storage/src/test/java/com/google/cloud/storage/ParallelCompositeUploadWritableByteChannelTest.java‎

Lines changed: 10 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@
3939
import com.google.cloud.storage.UnifiedOpts.ObjectSourceOpt;
4040
import com.google.cloud.storage.UnifiedOpts.ObjectTargetOpt;
4141
import com.google.cloud.storage.UnifiedOpts.Opts;
42-
import com.google.cloud.storage.otel.OpenTelemetryTraceUtil;
4342
import com.google.cloud.storage.spi.v1.StorageRpc;
4443
import com.google.common.base.MoreObjects;
4544
import com.google.common.base.Preconditions;
@@ -357,12 +356,9 @@ public void partsRetainMetadata() throws Exception {
357356
new FakeStorageInternal() {
358357
@Override
359358
public BlobInfo internalDirectUpload(
360-
BlobInfo info,
361-
Opts<ObjectTargetOpt> opts,
362-
ByteBuffer buf,
363-
OpenTelemetryTraceUtil.Context ctx) {
359+
BlobInfo info, Opts<ObjectTargetOpt> opts, ByteBuffer buf) {
364360
metadatas.add(info.getMetadata());
365-
return super.internalDirectUpload(info, opts, buf, null);
361+
return super.internalDirectUpload(info, opts, buf);
366362
}
367363

368364
@Override
@@ -450,10 +446,7 @@ public void creatingAnEmptyObjectWhichFailsIsSetAsResultFailureAndThrowFromClose
450446
new FakeStorageInternal() {
451447
@Override
452448
public BlobInfo internalDirectUpload(
453-
BlobInfo info,
454-
Opts<ObjectTargetOpt> opts,
455-
ByteBuffer buf,
456-
OpenTelemetryTraceUtil.Context ctx) {
449+
BlobInfo info, Opts<ObjectTargetOpt> opts, ByteBuffer buf) {
457450
throw StorageException.coalesce(
458451
ApiExceptionFactory.createException(
459452
null, GrpcStatusCode.of(Code.PERMISSION_DENIED), false));
@@ -564,10 +557,7 @@ public void shortCircuitExceptionResultsInFastFailure() throws Exception {
564557
new FakeStorageInternal() {
565558
@Override
566559
public BlobInfo internalDirectUpload(
567-
BlobInfo info,
568-
Opts<ObjectTargetOpt> opts,
569-
ByteBuffer buf,
570-
OpenTelemetryTraceUtil.Context ctx) {
560+
BlobInfo info, Opts<ObjectTargetOpt> opts, ByteBuffer buf) {
571561
if (induceFailure.getAndSet(false)) {
572562
Uninterruptibles.awaitUninterruptibly(blockForWrite1);
573563
try {
@@ -581,7 +571,7 @@ public BlobInfo internalDirectUpload(
581571
blockForWrite1Complete.countDown();
582572
}
583573
} else {
584-
return super.internalDirectUpload(info, opts, buf, null);
574+
return super.internalDirectUpload(info, opts, buf);
585575
}
586576
}
587577
};
@@ -724,11 +714,8 @@ public void partFailedPreconditionOnRetryIsHandledGracefully() throws Exception
724714
new FakeStorageInternal() {
725715
@Override
726716
public BlobInfo internalDirectUpload(
727-
BlobInfo info,
728-
Opts<ObjectTargetOpt> opts,
729-
ByteBuffer buf,
730-
OpenTelemetryTraceUtil.Context ctx) {
731-
BlobInfo blobInfo = super.internalDirectUpload(info, opts, buf, null);
717+
BlobInfo info, Opts<ObjectTargetOpt> opts, ByteBuffer buf) {
718+
BlobInfo blobInfo = super.internalDirectUpload(info, opts, buf);
732719
if (info.getName().equals(p1.getName())) {
733720
throw StorageException.coalesce(
734721
ApiExceptionFactory.createException(
@@ -791,10 +778,7 @@ public void partMetadataFieldDecoratorUsesCustomTime() throws IOException {
791778
new FakeStorageInternal() {
792779
@Override
793780
public BlobInfo internalDirectUpload(
794-
BlobInfo info,
795-
Opts<ObjectTargetOpt> opts,
796-
ByteBuffer buf,
797-
OpenTelemetryTraceUtil.Context ctx) {
781+
BlobInfo info, Opts<ObjectTargetOpt> opts, ByteBuffer buf) {
798782
if (info.getBlobId().getName().endsWith(".part")) {
799783
// Kinda hacky but since we are creating multiple parts we will use a range
800784
// to ensure the customTimes are being calculated appropriately
@@ -803,7 +787,7 @@ public BlobInfo internalDirectUpload(
803787
} else {
804788
assertThat(info.getCustomTimeOffsetDateTime()).isNull();
805789
}
806-
return super.internalDirectUpload(info, opts, buf, null);
790+
return super.internalDirectUpload(info, opts, buf);
807791
}
808792
};
809793
ParallelCompositeUploadWritableByteChannel pcu =
@@ -859,10 +843,7 @@ private static class FakeStorageInternal implements StorageInternal {
859843

860844
@Override
861845
public BlobInfo internalDirectUpload(
862-
BlobInfo info,
863-
Opts<ObjectTargetOpt> opts,
864-
ByteBuffer buf,
865-
OpenTelemetryTraceUtil.Context ctx) {
846+
BlobInfo info, Opts<ObjectTargetOpt> opts, ByteBuffer buf) {
866847
BlobId id = info.getBlobId();
867848

868849
BlobInfo.Builder b = info.toBuilder();

0 commit comments

Comments
 (0)