Skip to content

Census tracing integration for Firestore. #2729

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.

Sign up for GitHub

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

Jump to bottom
Merged
pongad merged 2 commits into from
Apr 26, 2018
Merged

Census tracing integration for Firestore. #2729

pongad merged 2 commits into from
Apr 26, 2018

Conversation

@danchia
Copy link

@danchia danchia commented Dec 20, 2017

@bogdandrutu PTAL. The dependency management was in particular quite nasty, and ideas / feedback around that area greatly appreciated.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 20, 2017
garrettjonesgoogle
garrettjonesgoogle reviewed Dec 20, 2017
View reviewed changes
google-cloud-firestore/pom.xml Outdated
<dependency>
<groupId>io.opencensus</groupId>
<artifactId>opencensus-api</artifactId>
<version>0.10.1</version>

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@danchia
Copy link
Author

danchia commented Dec 21, 2017

I updated the PR to use census from BOM instead.

bogdandrutu
bogdandrutu reviewed Feb 9, 2018
View reviewed changes
google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java Outdated

numResponses++;
if (numResponses == 1) {
Tracing.getTracer()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java Outdated
private static final String AUTO_ID_ALPHABET =
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";

public static final Tracer tracer = Tracing.getTracer();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java Outdated
span);
} else {
span.setStatus(
io.opencensus.trace.Status.ABORTED.withDescription("too many retries"));

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java Outdated

@Override
public void onError(Throwable throwable) {
Tracing.getTracer().getCurrentSpan().addAnnotation("Firestore.BatchGet: Error");

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java
numResponses++;
if (numResponses == 1) {
Tracing.getTracer()
.getCurrentSpan()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@danchia danchia requested a review from pongad as a code owner February 20, 2018 19:35
danchia
danchia commented Feb 20, 2018
View reviewed changes
Copy link
Author

@danchia danchia left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback, hope this addresses things.

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java Outdated
private static final String AUTO_ID_ALPHABET =
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";

public static final Tracer tracer = Tracing.getTracer();

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java Outdated

numResponses++;
if (numResponses == 1) {
Tracing.getTracer()

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java
numResponses++;
if (numResponses == 1) {
Tracing.getTracer()
.getCurrentSpan()

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java Outdated

@Override
public void onError(Throwable throwable) {
Tracing.getTracer().getCurrentSpan().addAnnotation("Firestore.BatchGet: Error");

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java Outdated
span);
} else {
span.setStatus(
io.opencensus.trace.Status.ABORTED.withDescription("too many retries"));

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/TraceUtil.java Outdated
import io.opencensus.trace.Status;

/** Census tracing utilities. */
class TraceUtil {

This comment was marked as spam.

Sign in to view

bogdandrutu
bogdandrutu reviewed Feb 26, 2018
View reviewed changes
google-cloud-firestore/src/main/java/com/google/cloud/firestore/TraceUtil.java Outdated
import io.opencensus.trace.Status;

/** Census tracing utilities. */
class TraceUtil {

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/TraceUtil.java Outdated
@@ -0,0 +1,35 @@
/*
* Copyright 2017 Google Inc. All Rights Reserved.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java
final Transaction.Function<T> transactionCallback,
final SettableApiFuture<T> resultFuture,
final TransactionOptions options) {
Span span = tracer.spanBuilder("CloudFirestore.Transaction").startSpan();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java Outdated
span);
} else {
span.setStatus(TOO_MANY_RETRIES_STATUS);
span.end();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

ApiStreamObserver<BatchGetDocumentsResponse> responseObserver =
new ApiStreamObserver<BatchGetDocumentsResponse>() {
int numResponses;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@danchia
Copy link
Author

danchia commented Mar 6, 2018

@pongad do you have any other feedback?

@pongad
Copy link
Contributor

pongad commented Apr 19, 2018

@danchia Sorry for the late response. I was transferring and dropped a bunch of things. This LGTM. @garrettjonesgoogle Do you have any other comments?

garrettjonesgoogle
garrettjonesgoogle approved these changes Apr 25, 2018
View reviewed changes
Copy link
Member

@garrettjonesgoogle garrettjonesgoogle left a comment

Choose a reason for hiding this comment

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

LGTM

@pongad pongad merged commit e7d5250 into googleapis:master Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pongad pongad Awaiting requested review from pongad

3 more reviewers

@vkedia vkedia vkedia left review comments

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@garrettjonesgoogle garrettjonesgoogle garrettjonesgoogle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@danchia @pongad @bogdandrutu @garrettjonesgoogle @vkedia @googlebot