-
Notifications
You must be signed in to change notification settings - Fork 434
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
Distributed tracing support with Micrometer and OpenTelemetry #376
Conversation
import io.opentelemetry.api.trace.Span; | ||
import io.opentelemetry.api.trace.TraceId; | ||
|
||
public class TracingClientInterceptor implements ClientInterceptor { |
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.
Should it be moved to grpc-client-spring-boot-starter?
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.
Yes
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.
done
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
public class LogInterceptor implements ServerInterceptor { |
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.
There is an example but LogInterceptor was not a part of grpc-spring-boot-starter. I can add it in a separate PR if needed
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 is this class needed ?
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.
Just wanted to use it from the library. Not sure why it doesn't exist. Removed
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
============================================
- Coverage 87.53% 85.35% -2.18%
Complexity 327 327
============================================
Files 51 54 +3
Lines 1612 1653 +41
Branches 96 99 +3
============================================
Hits 1411 1411
- Misses 159 200 +41
Partials 42 42
☔ View full report in Codecov by Sentry. |
@jvmlet Any thoughts? |
Sorry, busy days. Will review the PR soon |
|
||
@Configuration | ||
@EnableConfigurationProperties(OtlpProperties.class) | ||
public class OtelConfiguration { |
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 should be conditional on OpenTelemetrySdk
class
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.
makes sense
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
public class LogInterceptor implements ServerInterceptor { |
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 is this class needed ?
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.context.Scope; | ||
|
||
public class TracingServerInterceptor implements ServerInterceptor { |
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.
Should have the highest precedence https://github.com/LogNet/grpc-spring-boot-starter#421-interceptors-ordering
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.
added @order(20)? I don't see you use @Order
. Have you had in mind any other mechanisms?
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.
see this, we will need to move recovery
and auth
down 1
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.
I've set HIGHEST_PRECEDENCE. Do I need to touch recovery and auth here? Please, let me know if you want to change order somehow
@@ -286,6 +286,8 @@ dependencies { | |||
compileOnly "org.springframework.boot:spring-boot-starter-actuator" | |||
compileOnly "org.springframework.boot:spring-boot-starter-validation" | |||
compileOnly 'org.springframework.cloud:spring-cloud-starter-consul-discovery' | |||
compileOnly 'io.opentelemetry:opentelemetry-exporter-otlp:1.29.0' | |||
compileOnly 'io.micrometer:micrometer-tracing-bridge-otel:1.1.4' |
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.
io.micrometer:micrometer-tracing-bridge-otel
is it in-use ?
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.
probably better to use
|
Micrometer already supports gRPC instrumentation. So, using ObservationGrpcServerInterceptor and ObservationGrpcClientInterceptor from Micrometer is also an option |
@GRpcService(interceptors = [LogInterceptor::class, ObservationGrpcServerInterceptor::class]) That's what I tried from the beginning. But it leads to the error:
|
The issue can be avoided if we create a bean manually:
But not sure how to pass traceId and spanId for headers now |
ObservationGrpcServerInterceptor doesn't provide integration with SpanContext. It doesn't add headers into opentelemetry Context, so I can't log traceId and spanId |
Spring Boot supports both OpenTelemetry and OpenZipkin Brave for tracing. Micrometer is used as an abstraction layer over both libraries. I think grpc-spring-boot-starter should also support both tracing libraries, so only Micrometer API's should be used and no OpenTelemetry API's. |
yes, but I don't see in micrometer ability to send traces to gRPC collector |
Correct, because that isn't the responsibility of Micrometer, but the responsibility of one of the tracer implementations that can be chosen: https://docs.spring.io/spring-boot/docs/current/reference/html/actuator.html#actuator.micrometer-tracing.tracer-implementations |
Right, that's what I currently use. But I can't get traceId and spanId in logback
Probably, it's because of webflux: spring-projects/spring-framework#29466 |
No, I've tried everything. GrpcServerObservationContext is not propagated to GrpcServerObservationContext in GRpcService |
Context: #362