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

Add agent option for adjust ByteBuddy redefinition strategy #7887

Conversation

KyongSik-Yoon
Copy link

@laurit I make pr again due to missing cla commit from #7835

I'm doing maintanance javaagent of APM product.
Recently, We have scheduled accpet OpenTelemetry data from various agent of them.
So I tried install our javaagent with otel javaagent.
It's purpose that fill not enough funtionality of each of them.

My javaagent add interface to HttpServletRequest and HttpServletResponse loading them to free class reference when use their method as like getParameter()
But When otel javaagent installed sametimes interface added by my agent was gone.
That's look like return to origin shape.
I'm tried to found root cause and workaround then I found below issue.
#7594

A reason is otel javaagent using ByteBuddy redeifne strategy is RETRANSFORM
It's works that instrument byte code after finish class loading.
So redefined class ignored after doing retransform.

Surely, ByteBuddy has three redefinition strategy. (NONE, RETRANSFORM, REDEFINITION)
When I set REDEFINITION strategy to AgentInstaller, otel agent and my agent working well together.
But I'm not sure how affect to opentelemetry agent entire range soI added agent option can adjust redefinition strategy.
We can expect user who want to use multiple agent setting this option.

Signed-off-by: Yoon Kyong Sik <sam1287@gmail.com>
Signed-off-by: Yoon Kyong Sik <sam1287@gmail.com>
@KyongSik-Yoon KyongSik-Yoon requested a review from a team February 23, 2023 02:50
@KyongSik-Yoon
Copy link
Author

KyongSik-Yoon commented Feb 23, 2023

@laurit @trask
With this it would be possible to pass as the redefinition strategy that would break the agent, not sure if we should allow passing options that will not result in functioning agent.

-> I will add assertion check for passing strategy should be redefinition or retransform.

I find it strange that works while DISABLEDREDEFINITIONRETRANSFORM doesn't. These should affect only the initial transform of already loaded classes

-> I don't have experience working with ByteBuddy. When I use bytecode instrumentation lib as like BCEL, ASM, Javassit I didn't saw like this situation. As I know bytecode handling should not affect metadata of already loaded classes.

From your description it seems that when using the missing interface issue happens only sometimes, is this true?

-> In my case, missing interface issue always happend when use otel agent and own.
Three is one strange things that although configure REDEFINITION missing interface possible happended by javaagent configuration order. Everything is ok just when otel javaagent setting position before than other javaagent.
ex) -javaagent:otel.javaagent.jar -javaagent:other.javaagent.jar
In other case, missing interface happended again. I think it's caused by ByteBuddy's RETRANSFORM

Any chance of building a reproducer for this?

-> I'wll try make this case using unit test. If it's not possible I will attach a part of using code for adding interface.

@KyongSik-Yoon
Copy link
Author

@laurit It's not easy regenerate this case using unit test code.
I attached code for add interface to http servlet request.

public class HttpServletRequestVisitor extends ClassWrapper
{
    public HttpServletRequestVisitor(ClassWrapper cv)
    {
        this.cv = cv;
    }

    /**
     * (non-Javadoc)
     *
     * @see ClassWrapper#visit(int, int, String, String, String, String[])
     */
    public void visit(int version,
                      int access,
                      String name,
                      String signature,
                      String superName,
                      String[] interfaces)
    {
        super.visit(version, access, name, signature, superName, interfaces);

        String[] holding = new String[interfaces.length + 1];
        holding[holding.length - 1] = "aries/base/HttpServletRequest";
        System.arraycopy(interfaces, 0, holding, 0, interfaces.length);

        cv.visit(version, access, name, signature, superName, holding);
    }
}

Missing interace is aries/base/HttpServletRequest when use otel and other agent.
When use alone agent my own or with otel agent configured redefinition strategy added interface remained expectedly.
If you want I can make simple agent just do add interface then I think you can't reproduce this problem.
I'll wait your reply.

Signed-off-by: Yoon Kyong Sik <sam1287@gmail.com>
@KyongSik-Yoon
Copy link
Author

@laurit I prepare two repositories for reproduce missing interface.

Agent

https://github.com/KyongSik-Yoon/simple-javaagent

Http server using javalin

https://github.com/KyongSik-Yoon/missing-interface

How to reproduce

Run server with upon agent and otel agent (javaagent order is important.)

java -javaagent:C:\Users\sam12\test\otel\opentelemetry-javaagent.jar -javaagent:C:\Users\sam12\dev\git-repo\simple-javaagent\build\libs\simple-javaagent-1.0-SNAPSHOT-all.jar -jar .\missing-interface-1.0-SNAPSHOT-all.jar

Call url from a console or browser

http://localhost:7070/interface-of/jakarta.servlet.http.HttpServletRequest

Response

When use only my agent

[interface jakarta.servlet.ServletRequest, interface simple.javaagent.MyInterface]

When use otel and my agent

[interface jakarta.servlet.ServletRequest]

@KyongSik-Yoon
Copy link
Author

opentelemetry-javaagent-1.23.0-SNAPSHOT.zip
This zip contains otel agent built by me. (strategy fixed to REDEFINITION)

@laurit
Copy link
Contributor

laurit commented Feb 27, 2023

@KyongSik-Yoon Thanks for the reproducer. Hopefully fixed with #7916

@KyongSik-Yoon
Copy link
Author

@laurit I was build your branch and check your patch fixed this issue.
Your commits applied in this scenario.
If otel agent don't have plan provide redefinition optionally, possible close this issue.

@trask trask closed this Feb 28, 2023
laurit added a commit that referenced this pull request Mar 1, 2023
Fixes the issue described in
#7887
Hopefully resolves
#7594
We should not use cached `TypeDescription` for currently transformed
class as the cached description will not be the same as newly created
one if the class bytes were transformed. For example if another agent
adds an interface to the class then returning the cached description
that does not have that interface would result in bytebuddy removing
that interface, see
https://github.com/raphw/byte-buddy/blob/665a090c733c37339788cc5a1c441bd47fb77ef0/byte-buddy-dep/src/main/java/net/bytebuddy/dynamic/scaffold/TypeWriter.java#L5012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants