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

Client span missed when Armeria GRPC sever A call Armeria GRPC sever B #9726

Closed
cxjava opened this issue Oct 20, 2023 · 8 comments · Fixed by #11351
Closed

Client span missed when Armeria GRPC sever A call Armeria GRPC sever B #9726

cxjava opened this issue Oct 20, 2023 · 8 comments · Fixed by #11351
Labels
contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome enhancement New feature or request new instrumentation

Comments

@cxjava
Copy link

cxjava commented Oct 20, 2023

Describe the bug

When user have two Armeria GRPC services: Armeria GRPC services A and Armeria GRPC services B

if the call chain isweb client -> Service A's GRPC method -> Service B's GRPC method

The client span is missed, server A span -> client span(❌missed❌) -> server B span

server A and server B have different trace id.

Steps to reproduce

Reproduce it in the armeria grpc example project.

copy the grpc folder to grpc2.

for original grpc folder, don't need to do too much changes, just need add the gradle setting:

application {
    mainClass.set('example.armeria.grpc.Main')
}

applicationDefaultJvmArgs = ["-javaagent:/Users/user/Downloads/opentelemetry-javaagent.jar",
"-Dotel.resource.attributes=service.name=grpc",
"-Dotel.javaagent.debug=true",
"-Dotel.metrics.exporter=none",
"-Dotel.traces.exporter=jaeger"
]

=======================
for grpc2 folder, need update the first method in HelloServiceImpl:

 @Override
    public void hello(HelloRequest request, StreamObserver<HelloReply> responseObserver) {
        var client = GrpcClients.newClient("http://127.0.0.1:8080/",
                HelloServiceBlockingStub.class);
        var message = client.hello(HelloRequest.newBuilder().setName("client").build()).getMessage();
        if (request.getName().isEmpty()) {
            responseObserver.onError(
                    Status.FAILED_PRECONDITION.withDescription("Name cannot be empty").asRuntimeException());
        } else {
            responseObserver.onNext(buildReply(toMessage(request.getName() + message)));
            responseObserver.onCompleted();
        }
    }

update default port in main method:

public static void main(String[] args) throws Exception {
        final Server server = newServer(8180, 8143);

        server.closeOnJvmShutdown();

        server.start().join();

        logger.info("Server has been started. Serving DocService at http://127.0.0.1:{}/docs",
                    server.activeLocalPort());
    }

update the default JVM args:

application {
    mainClass.set('example.armeria.grpc.Main')
}

applicationDefaultJvmArgs = ["-javaagent:/Users/user/Downloads/opentelemetry-javaagent.jar",
"-Dotel.resource.attributes=service.name=grpc2",
"-Dotel.javaagent.debug=true",
"-Dotel.metrics.exporter=none",
"-Dotel.traces.exporter=jaeger"
]

When call localhost:8180 the response is :

{
	"message": "Hello, TestHello, client!!"
}

Expected behavior

Can find the two span in JEAGER:

image image

Actual behavior

but only find the one span in each service:
grpc service:
image

grpc2 service:
image

Javaagent or library instrumentation version

1.31.0

Environment

JDK: corretto-17.0.6
OS: macOS 13.6

Armeria: 1.25.2

Jaeger v1.48.0

Additional context

@laurit maybe need to do some enhancements like this PR
CC: @jrhee17

@cxjava cxjava added bug Something isn't working needs triage New issue that requires triage labels Oct 20, 2023
@laurit laurit added enhancement New feature or request contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome and removed needs triage New issue that requires triage labels Oct 20, 2023
@laurit
Copy link
Contributor

laurit commented Oct 20, 2023

Probably requires adding instrumentation for armeria grcp client/server.

@laurit laurit added new instrumentation and removed bug Something isn't working labels Oct 20, 2023
@jrhee17
Copy link

jrhee17 commented Oct 23, 2023

One possible quick fix may be to change the advice location to AbstractClientOptionsBuilder#buildOptions

main...jrhee17:opentelemetry-java-instrumentation:feature/support-armeria-grpc

@ikhoon
Copy link

ikhoon commented Oct 24, 2023

@cxjava
Copy link
Author

cxjava commented Jan 2, 2024

@laurit Is there any plan to add instrumentation for armeria grcp client/server base on jrhee17 andikhoon's comment ?

@laurit
Copy link
Contributor

laurit commented Jan 2, 2024

@cxjava Have you tried using grpc instrumentation from https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/grpc-1.6/library as @ikhoon suggested? Perhaps a dedicated instrumentation isn't needed at all?

@laurit laurit added the needs author feedback Waiting for additional feedback from the author label Jan 2, 2024
@cxjava
Copy link
Author

cxjava commented Jan 3, 2024

After add the dependency in client and server side:


    implementation 'io.opentelemetry:opentelemetry-api'
    implementation 'io.opentelemetry:opentelemetry-sdk'
    implementation 'io.opentelemetry:opentelemetry-sdk-metrics'
    implementation 'io.opentelemetry:opentelemetry-exporter-otlp'
    implementation 'io.opentelemetry:opentelemetry-exporter-logging'
    implementation 'io.opentelemetry:opentelemetry-exporter-jaeger'
    runtimeOnly 'io.opentelemetry:opentelemetry-semconv:1.30.1-alpha'
    implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
    implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure-spi'
    implementation 'io.opentelemetry.instrumentation:opentelemetry-grpc-1.6'

client:

 @Bean
    public OpenTelemetry openTelemetry() {
        return AutoConfiguredOpenTelemetrySdk.initialize().getOpenTelemetrySdk();
    }

    @Bean
    public AddressServiceGrpc.AddressServiceBlockingStub addressServiceBlockingStub(OpenTelemetry openTelemetry) {
        return GrpcClients.builder("xxxx")
                .decorator(LoggingClient.newDecorator())
                .intercept(GrpcTelemetry.create(openTelemetry).newClientInterceptor())
                .build(AddressServiceGrpc.AddressServiceBlockingStub.class);
    }

server:

 @Bean
    public OpenTelemetry openTelemetry() {
        return AutoConfiguredOpenTelemetrySdk.initialize().getOpenTelemetrySdk();
    }

 GrpcService grpcService = GrpcService.builder()
                .addService(addressService)
                .intercept(GrpcTelemetry.create(openTelemetry).newServerInterceptor())
                .enableUnframedRequests(true)
                .build();

run without the open telemetry java agent, it works fine as expected.

image

Other question is that: can java agent support it? Normally we use the java agent more frequently than the dependency.

I also try to add some configuration like -Dotel.instrumentation.grpc.enabled=true when use the java gent. But it failed(can't find the two span in one trace id).

@github-actions github-actions bot removed the needs author feedback Waiting for additional feedback from the author label Jan 3, 2024
@cxjava
Copy link
Author

cxjava commented Jan 17, 2024

Hi @laurit, do you have any concern about open telemetry java agent support for armeria grcp client/server not only open telemetry dependency?

@cxjava
Copy link
Author

cxjava commented May 11, 2024

Hi @laurit ,

I hope this email finds you well. I would like to propose this feature again that could greatly benefit this project and enhance opentelemetry observability capabilities.

As you may be aware, I have recently encountered a scenario where we need to leverage the power of OpenTelemetry's Java Agent. However, I do not anticipate the need to introduce dependencies such as implementation 'io.opentelemetry:opentelemetry-exporter-otlp'. While this dependency allows for seamless traceID propagation between Armeria GRPC services, our project's unique requirements call for a more tailored approach.

There is an implementation contributed by @jrhee17 that perfectly aligns with our needs. This implementation allows us to utilize the OpenTelemetry Java Agent without the need for additional dependencies. I have successfully built and tested the jar locally, and it fulfills our requirements flawlessly.

One possible quick fix may be to change the advice location to AbstractClientOptionsBuilder#buildOptions

main...jrhee17:opentelemetry-java-instrumentation:feature/support-armeria-grpc

By adopting jrhee17's implementation, we can benefit from enhanced observability capabilities while maintaining a lean and efficient codebase. This approach aligns with our project's principles of minimizing external dependencies and promoting code clarity.

I kindly request your consideration and approval to merge jrhee17's implementation into this codebase. This addition will not only streamline our development process but also ensure that we maintain a high level of code quality and maintainability.

I believe that other Armeria users also hope that this becomes a natively supported feature for OpenTelemetry Java Agent.

Please let me know if you require any further information or have any concerns regarding this. I would be happy to provide additional details or clarify any aspects of the proposed feature.

Thank you for your time and consideration!

Best regards

the screenshot and opentelemetry java agent
  • successful screenshot
Snipaste_2024-05-11_16-27-37
  • java agent build from jrhee17's implementation
    java agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome enhancement New feature or request new instrumentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants