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

Support for ArmeriaRpcClientBuilderInstrumentation #6319

Closed
jmadureira opened this issue Jul 13, 2022 · 6 comments · Fixed by #11351
Closed

Support for ArmeriaRpcClientBuilderInstrumentation #6319

jmadureira opened this issue Jul 13, 2022 · 6 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

@jmadureira
Copy link

jmadureira commented Jul 13, 2022

Is your feature request related to a problem? Please describe.
The current open telemetry javaagent won't trace Armeria RPC client calls because the respective instrumentation doesn't exit.

Describe the solution you'd like
I was able to test that the current clientDecorator returned by the ArmeriaTelemetry class works as expected when decorating gRPC clients:

// Just a hack to get the info from the GlobalOpenTelemetry
ArmeriaTelemetry telemetry = ArmeriaTelemetry.create(GlobalOpenTelemetry.get());

// Adding the default client decorator won't cause any compilation errors
var client = GrpcClients.builder(uri()).decorator(telemetry.newClientDecorator()).build(SomeGrpcStub.class);

As such I'm assuming that the only thing required is to create another Intrumentation for the com.linecorp.armeria.client.grpc.GrpcClientBuilder following the same structure used by the ArmeriaWebClientBuilderInstrumentation

/*
 * Copyright The OpenTelemetry Authors
 * SPDX-License-Identifier: Apache-2.0
 */

package io.opentelemetry.javaagent.instrumentation.armeria.v1_3;

import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.named;

import com.linecorp.armeria.client.grpc.GrpcClientBuilder;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

public class ArmeriaRpcClientBuilderInstrumentation implements TypeInstrumentation {

  @Override
  public ElementMatcher<TypeDescription> typeMatcher() {
    // we need to update class being targeted
    return named("com.linecorp.armeria.client.grpc.GrpcClientBuilder");
  }

  @Override
  public void transform(TypeTransformer transformer) {
    transformer.applyAdviceToMethod(
        isMethod().and(isPublic()).and(named("build")),
        // reference this class' build advice
        ArmeriaRpcClientBuilderInstrumentation.class.getName() + "$BuildAdvice");
  }

  @SuppressWarnings("unused")
  public static class BuildAdvice {

    @Advice.OnMethodEnter
    // the the GrpcClientBuilder instead
    public static void build(@Advice.This GrpcClientBuilder builder) {
      builder.decorator(ArmeriaSingletons.CLIENT_DECORATOR);
    }
  }
}

BTW I'm not 100% certain that this will work because I'm not familiar enough with the SDK and haven't had enough time to play with it.

Describe alternatives you've considered
The only alternative I've considered was the previous example I mentioned where the GrpcClientBuilder was manually decorated.

Additional context
N/A

@jmadureira jmadureira added the enhancement New feature or request label Jul 13, 2022
@mateuszrzeszutek
Copy link
Member

Hey @jmadureira ,
We already have a gRPC instrumentation (plain gRPC, not the Armeria wrapper): https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/grpc-1.6
Does it not work for you?

@jmadureira
Copy link
Author

jmadureira commented Jul 13, 2022

Just noticed that the com.linecorp.armeria.client.grpc.GrpcClientBuilder doesn't exist in Armeria version 1.3.0 so I suppose it will depend the willingness and availability to update to some later version. The current version btw is 1.17.1 and the com.linecorp.armeria.client.grpc.GrpcClientBuilder seems to have been added in 1.14.0.

Thanks for the reply @mateuszrzeszutek but unfortunately the already existing gRPC instrumentation doesn't seem to work. My assumption is that the gRPC instrumentation works at the io.grpc.ManagedChannelBuilder level only which isn't used by Armeria.

@mateuszrzeszutek
Copy link
Member

Thanks for the reply @mateuszrzeszutek but unfortunately the already existing gRPC instrumentation doesn't seem to work. My assumption is that the gRPC instrumentation works at the io.grpc.ManagedChannelBuilder level only which isn't used by Armeria.

Yes, that seems to be correct. Looks like we'll need that additional instrumentation after all.

I was able to test that the current clientDecorator returned by the ArmeriaTelemetry class works as expected when decorating gRPC clients:

I think that rather than applying a HTTP decorator, we'll need to add a gRPC ClientInterceptor (from GrpcTelemetry), as it implements the RPC semantic conventions.

@mateuszrzeszutek mateuszrzeszutek added contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome new instrumentation labels Jul 13, 2022
@jmadureira
Copy link
Author

I think that rather than applying a HTTP decorator, we'll need to add a gRPC ClientInterceptor (from GrpcTelemetry), as it implements the RPC semantic conventions.

I'm actually kinda curious in knowing how this can be solved at the gRPC level. Assuming that the io.grpc.ManagedChannelBuilder cannot be used anymore becuse it won't work with Armeria what other entrypoint is there where a ClientInterceptor can be injected?

@jmadureira
Copy link
Author

I think you can add them here: https://github.com/line/armeria/blob/master/grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java#L338

Ah ok so the idea is still to instrument at the Armeria level, only instead of creating a HTTP decorator we create a gRPC ClientInterceptor instead with the same behaviour as the ClientInterceptor defined at the GrpcTelemetry.

I could take a shot at solving this if #6242 was fixed. Otherwise it will be a shot in the dark.

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.

2 participants