Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add 'isTransactional' attribute. #1657

Merged
merged 1 commit into from
May 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import com.google.cloud.firestore.telemetry.TraceUtil;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.firestore.v1.BatchGetDocumentsRequest;
import com.google.firestore.v1.BatchGetDocumentsResponse;
import com.google.firestore.v1.DatabaseRootName;
Expand Down Expand Up @@ -235,7 +236,10 @@ public void onStart(StreamController streamController) {
.currentSpan()
.addEvent(
TraceUtil.SPAN_NAME_BATCH_GET_DOCUMENTS + ": Start",
Collections.singletonMap("numDocuments", documentReferences.length));
new ImmutableMap.Builder<String, Object>()
.put("numDocuments", documentReferences.length)
.put("isTransactional", transactionId != null)
.build());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,8 @@ ApiFuture<List<WriteResult>> commit(@Nullable ByteString transactionId) {
? TraceUtil.SPAN_NAME_BATCH_COMMIT
: TraceUtil.SPAN_NAME_TRANSACTION_COMMIT);
span.setAttribute("numDocuments", writes.size());
span.setAttribute("isTransactional", transactionId != null);

try (Scope ignored = span.makeCurrent()) {
ApiFuture<CommitResponse> response =
firestore.sendRequest(request.build(), firestore.getClient().commitCallable());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ public TraceUtil.Span setAttribute(String key, String value) {
return this;
}

@Override
public TraceUtil.Span setAttribute(String key, boolean value) {
return this;
}

@Override
public Scope makeCurrent() {
return new Scope();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,12 @@ public TraceUtil.Span setAttribute(String key, String value) {
return this;
}

@Override
public TraceUtil.Span setAttribute(String key, boolean value) {
span.setAttribute(ATTRIBUTE_SERVICE_PREFIX + key, value);
return this;
}

@Override
public Scope makeCurrent() {
return new Scope(span.makeCurrent());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ interface Span {
/** Adds the given attribute to this span. */
Span setAttribute(String key, String value);

/** Adds the given attribute to this span. */
Span setAttribute(String key, boolean value);

/** Marks this span as the current span. */
Scope makeCurrent();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ private boolean dfsContainsCallStack(long spanId, List<String> expectedCallStack
if (spanName(spanId).equals(expectedCallStack.get(0))) {
// Recursion termination
if (childSpans(spanId) == null) {
logger.info("No more chilren for " + spanName(spanId));
logger.info("No more children for " + spanName(spanId));
return true;
} else {
// Examine the child spans
Expand Down Expand Up @@ -241,7 +241,7 @@ private boolean dfsContainsCallStack(long spanId, List<String> expectedCallStack

private static final int NUM_SPAN_ID_BYTES = 16;

private static final int GET_TRACE_RETRY_COUNT = 15;
private static final int GET_TRACE_RETRY_COUNT = 60;
jimit-j-shah marked this conversation as resolved.
Show resolved Hide resolved

private static final int GET_TRACE_RETRY_BACKOFF_MILLIS = 1000;

Expand Down Expand Up @@ -513,10 +513,7 @@ protected void fetchAndValidateTrace(
// For Non-Transaction traces, there is a 1:1 ratio of spans in `spanNames` and in the trace.
protected void fetchAndValidateTrace(String traceId, String... spanNames)
throws InterruptedException {
int numRetries = GET_TRACE_RETRY_COUNT;
int numSpans = spanNames.length;

fetchAndValidateTrace(traceId, numSpans, Arrays.asList(Arrays.asList(spanNames)));
fetchAndValidateTrace(traceId, spanNames.length, Arrays.asList(Arrays.asList(spanNames)));
}

@Test
Expand All @@ -541,15 +538,24 @@ public void traceContainerTest() throws Exception {
try {
traceResp = traceClient_v1.getTrace(projectId, customSpanContext.getTraceId());
if (traceResp.getSpansCount() == expectedSpanCount) {
logger.info("Success: Got " + expectedSpanCount + " spans.");
break;
}
} catch (NotFoundException notFoundException) {
Thread.sleep(GET_TRACE_RETRY_BACKOFF_MILLIS);
logger.info("Trace not found, retrying in " + GET_TRACE_RETRY_BACKOFF_MILLIS + " ms");
}
logger.info(
"Trace Found. The trace did not contain "
+ expectedSpanCount
+ " spans. Going to retry.");
numRetries--;
} while (numRetries > 0);

// Make sure we got as many spans as we expected.
assertNotNull(traceResp);
assertEquals(expectedSpanCount, traceResp.getSpansCount());

TraceContainer traceCont = new TraceContainer(rootSpanName, traceResp);

// Contains exact path
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -580,10 +580,13 @@ public void queryGet() throws Exception {
"RunQuery",
Attributes.builder()
.put("isRetryRequestWithCursor", false)
.put("transactional", false)
.put("isTransactional", false)
.build()));
assertTrue(
hasEvent(span, "RunQuery: Completed", Attributes.builder().put("numDocuments", 0).build()));
hasEvent(
getGrpcSpanByName(RUN_QUERY_RPC_NAME),
"RunQuery: Completed",
Attributes.builder().put("numDocuments", 0).build()));
}

@Test
Expand Down Expand Up @@ -733,6 +736,12 @@ public void writeBatch() throws Exception {
List<SpanData> spans = prepareSpans();
assertEquals(2, spans.size());
assertSpanHierarchy(SPAN_NAME_BATCH_COMMIT, grpcSpanName(COMMIT_RPC_NAME));
assertEquals(
false,
getSpanByName(SPAN_NAME_BATCH_COMMIT)
.getAttributes()
.get(AttributeKey.booleanKey("gcp.firestore.isTransactional"))
.booleanValue());
assertEquals(
3L,
getSpanByName(SPAN_NAME_BATCH_COMMIT)
Expand Down
Loading