spring-attic / spring-cloud-gcp

Integration for Google Cloud Platform APIs with Spring
Apache License 2.0
706 stars 693 forks source link

Bug: GCP related common sense errors are not reported to ERROR log #1752

Open bbzg opened 5 years ago

bbzg commented 5 years ago

Today I wasted an inordinate amount of time to figure out why a new project was not reporting spans to Stackdriver Trace.

The error message, which was clear as day, was logged as DEBUG by zipkin2.reporter.AsyncReporter$BoundedAsyncReporter

The error that was logged was (formatted for readability):

z.r.AsyncReporter$BoundedAsyncReporter:
    Dropped 2 spans due to StatusRuntimeException(
        PERMISSION_DENIED: Stackdriver Trace API has not been used in project <my project id> before or it is disabled. 
        Enable it by visiting https://console.developers.google.com/apis/api/cloudtrace.googleapis.com/overview?project=<my project id> then retry. 
        If you enabled this API recently, wait a few minutes for the action to propagate to our systems and retry.
    )

Which tells me exactly what I need to do, but who in their right mind sets their tracing library to DEBUG level (except when you have tried everything).

This is surely an error and should be logged with an ERROR level?

Thank you for an otherwise excellent product! =)

meltsufin commented 5 years ago

@bbzg Are you using our Trace starter? Which version of spring-cloud-gcp are you using? The log statement seems to be coming from zipkin-reporter-java. I don't believe Spring Cloud GCP even has a dependency on this module.

bbzg commented 5 years ago

This is what I am using:

The library that configures the tracing:

    implementation 'org.springframework.cloud:spring-cloud-starter-sleuth'
    implementation 'org.springframework.cloud:spring-cloud-gcp-starter-trace'
    implementation 'io.zipkin.brave:brave-instrumentation-okhttp3'

And then these boms

    dependencyManagement {
        imports {
            mavenBom 'org.springframework.boot:spring-boot-dependencies:2.1.5.RELEASE'
            mavenBom 'com.google.cloud:google-cloud-bom:0.94.0-alpha'
            mavenBom 'org.springframework.cloud:spring-cloud-dependencies:Greenwich.RELEASE'
        }

         dependencies {
            dependency 'io.zipkin.brave:brave-instrumentation-okhttp3:5.6.1'
            dependency 'io.zipkin.brave:brave-instrumentation-mysql:5.6.1'
        }

Why okhttp/mysql aren't included is a story of its own, but ignore that for now I guess.

I know that there are newer versions of the dependencies, but would that really have helped?

meltsufin commented 5 years ago

Looks like it's coming from zipkin-gcp. In any case, the code is not in this repo. So, we can't really fix it here. Can you file the bug in the zipkin-gcp repository? Thanks!

dzou commented 5 years ago

@bbzg - Hey, I filed a bug for you with Zipkin here: https://github.com/openzipkin/zipkin-gcp/issues/129

We will wait to see the Zipkin team's response and if there might be a fix for this issue.

bbzg commented 5 years ago

Thank you @meltsufin and @dzou, I appreciate your hard work!

codefromthecrypt commented 5 years ago

a better way to solve this problem is to not wait until runtime to find it. I opened this in sleuth about that https://github.com/spring-cloud/spring-cloud-sleuth/issues/1411

if there is a flaw in the health check of the stackdriver sender, better to focus energy there in making it more robust

codefromthecrypt commented 5 years ago

FWIW I think probably 1/100 people even know there's a health check function on all senders :P

meltsufin commented 5 years ago

Here's the implementation of the health check function in StackdriverSender:

  public CheckResult check() {
    return CheckResult.OK;
  }

Not particularly useful. :smile:

codefromthecrypt commented 5 years ago

@meltsufin maybe raise a pull request? not all senders have great ones but most have something better than this. As this code is literally for a commercial product it could help that one of the vendors helps fix that vs expecting labor from volunteers for everything

meltsufin commented 5 years ago

@adriancole Sure, we can look into it.

elefeint commented 5 years ago

zipkin-gcp is now fully set up to report errors. spring-cloud-gcp can take advantage of the enhancements once we upgrade (tracked in #1862 and #1786).