9 Replies Latest reply on Feb 1, 2017 10:12 AM by ochaloup

    Dtest library change to support INTERFACE in any Intrumentor method

    ochaloup

      Hi Andrew,

       

      I've propsed another change for Dtest library where I would like to have chance to use globaly not only CLASS hook but INTERFACE as wel..

      My proposal is now done in PR: https://github.com/bytemanproject/byteman/pull/46

       

      But I would like to discuss it with you. Basically I want two things from Instrumentor

      • being able to use INTERFACE and CLASS
      • not necessarily to have a class on classpath - being able to provide class as a string only  format for instrumentation

       

      I've started to do it by checking if it's interface directly in the Instrumentor code. I would say there could be other options

      • checks in Instrumentor if class is instantiable - that was my first idea but when used overall the class it seems to be a bit chatty
      • checks in RuleBuilder - my feeling was this is only a DTO and so such functionality does not belong there, but it would be nicely packed in one class
      • add methods like injectOnCallInterface, injectOnMethodExitInterface, injctOnMethodEntryInterface... - personally I do not like this but it has advantage that I don't need to have any facility of checking if class in loadable by classloader

      ...maybe some other design proposal?

       

      Plus I'm thinking of adding somehow a way to say that RuleBuilder would use '^' to define that rule should be executed on class/interface and all his children. Currently thinking how to do it in nice way.

       

      What do you think? Is the PR acceptable, what would you think should be done otherwise?

       

      Thank you

      Ondra

        • 1. Re: Dtest library change to support INTERFACE in any Intrumentor method
          adinn

          Hi Ondrej,

           

          Before discussing possible implementations I'd like to understand and clarify what the requirement is here.

           

          At present dtest is designed to provide an audit trail for invocations of some subset of the methods of an application and, possibly, the underlying middleware or Java runtime libraries that the app relies on. As a consequence, the rules constructed by the Injector are specific to a given class and method and they add a record of a call to that class+method combination to the audit i.e. they record the call event at the lowest possible level of granularity. Similarly, the back end records audit events at the same level of granularity (class InstrumentedInstance) grouping them by owner class (class InstrumentedClass).

           

          So, what is the purpose and expected behaviour when you instrument an interface?

           

          Any rules you inject will be injected into one or more specific classes. One possible behaviour is for events to be audited simply as calls to a method of the interface, ignoring any details of the underlying class (n.b. this would require adding a registry stub for the interface and using it to receive the trace messages for the interface). The result is to group call records at a higher level of granularity than would be achieved by instrumenting the classes themselves.

           

          Alternatively, the interface could be used simply as a convenience to instrument any underlying implementation classes of the interface in the same way as the existing instrumentor does i.e. to record calls at the granularity of implementing class plus method.

           

          [ an aside  regarding implementation:

           

          It is easy to implement rules to work this way. The generated rules simply have to pass the name of the trigger class -- i.e. the class the rule is actually injected into -- rather than the target class (strictly interface) -- i.e. the class named in the CLASS (INTERFACE) clause --  to the trace call. The special variable $CLASS can be used to reference the trigger class name). However, this model poses a problem at generate time interface is merely named rather than provided as a Class<?> instance. How do you add registry entries to field the trace calls for any implementing classes without knowing what those classes are in advance?

          ]

           

          This alternative behaviour might be a tad confusing if the user inadvertently instruments a method via an interface and also using a target class-specific rule. The outcome would be that calls would get audited twice to the same destination, once by the interface rule and once by the class rule. The same confusion might arise when a class implements multiple interfaces and rules are injected via those interfaces. By contrast, the first behaviour would also record the call multiple times but would do so in distinct parts of the audit trail, under the relevant interface or class.

           

          A variant of the first option is to have interface rules trace both the target interface and trigger class as well as the method name. So, the audit would group calls by interface + method but also permit subgrouping by interface + method + implementing class. This is interesting because it allows the per-class audit to be correlated with the per-interface audit but still keeps the records independently indexed.

           

          So, do any of these behaviours cover your intended use case? If so then which? If not then what is it you want that is not covered here?

           

          regards,

           

          Andrew Dinn

          • 2. Re: Dtest library change to support INTERFACE in any Intrumentor method
            ochaloup

            Hi Andrew,

             

            I'm sorry for confusion. I haven't put my expectation well here. You are talking about functionality of instrumenting the class by Instrumentor (method instrumentClass). I do use it and I'm perfectly satisfied with it. My understanding of the functionality of Instrumentor is double from this point of view.

             

            First it's capable to provide audit trail as you describes. That's perfect and I do use it to my whole satisfaction.

            The second part is java API for injecting rules. They're methods like injectOnCall or crashAtMethod[Entry|Exit|...].

             

            My point of this discussion is only about the second usage of the Instrumentor class. I use it for injecting rules through java API sending those rules to a remote JVM. I would like to add an ability of dtest to work with class and interface when I call method like "crashAtMethod". The second point is that I would like to use string as class name and not necessarily to pass Class to the API.

             

            Does that get a bit more sense now?

            • 3. Re: Dtest library change to support INTERFACE in any Intrumentor method
              adinn

              Hi Ondra,

              Ondřej Chaloupka wrote:

               

              My point of this discussion is only about the second usage of the Instrumentor class. I use it for injecting rules through java API sending those rules to a remote JVM. I would like to add an ability of dtest to work with class and interface when I call method like "crashAtMethod". The second point is that I would like to use string as class name and not necessarily to pass Class to the API.

               

              Does that get a bit more sense now?

               

              Yes, and apologies for the confusion which was, I think, mostly on my part.

               

              So, the interface we currently have for requesting instrumentation to be injected is the following

               

              // group A
              InstrumentedClass instrumentClass(Class clazz)
              InstrumentedClass instrumentClass(Class clazz, Set<String> methodNames)
              InstrumentedClass instrumentClass(String className, Set<String> methodNames)
              
              // group B
              void injectOnCall(Class clazz, String methodName, String action)
              void injectOnExit(Class clazz, String methodName, String action)
              void injectOnMethod(Class clazz, String methodName, String condition, String action, String where)
              
              // group C
              injectFault(Class clazz, String methodName, Class<? extends Throwable> fault, Object[] faultArgs)
              
              // group D
              void crashAtMethodExit(Class clazz, String methodName)
              void crashAtMethodEntry(Class clazz, String methodName)
              void crashAtMethodEntry(String className, String methodName)
              void crashAtMethod(String className, String methodName, String where)
              
              

               

              n.b. the API is incomplete at line 11 where to be consistent there really ought to be

               

              void crashAtMethodExit(String className, String methodName)
              
              
              
              
              
              
              
              

               

              This is not critical since you can achieve the desired effect by calling

               

                crashAtMethod(className, methodName, "EXIT")
              
              
              
              
              
              
              
              

               

              However, we probably ought to remedy that omission.

               

              So, your question seems to be how do we generalize this interface so that it extends to fully support injection into interfaces -- including the case where we only kn ow the interface name and also supports injection down hierarchies?

               

              Also, we must retain backwards compatibility in any new API we come up with.

               

              I think it is best to address this at the full level of generality rather than do it piecemeal.

               

              The API actually displays a hiererachical structure, certain calls being built on others e.g.

              instrumentClass(Class clazz)

              calls

              instrumentClass(Class clazz, Set<String> methodNames)

              calls

              instrumentClass(String className, Set<String> methodNames).

               

              The same applies for groups B and D. The calls at the bottom employ String names rather than Class<?> instances.

               

              We can profit by this when we extend the API (including modifying group C so that it also conforms to the same pattern). We need the methods at the bottom to accept String names and an argument or arguments to indicate whether we are injecting into a CLASS or INTERFACE and whether we are injecting directly or down a hierarchy. So, at the bottom of each group we need a base method which does the bulk of the real work i.e.

               

              // Group A
              InstrumentedClass instrument(String className, Set<String> methodNames, boolean isInterface, boolean includeSubclasses)
              
              // Group B
              void injectOnMethod(String classname, String methodName, String condition, String action, String where, boolean isInterface, boolean includeSubclasses)
              
              // Group C
              injectFault(String className, String methodName, Class<? extends Throwable> fault, Object[] faultArgs, boolean isInterface, boolean includeSubclasses)
              
              // Group D
              void crashAtMethod(String className, String methodName, String where, boolean isInterface, boolean includeSubclasses)
              
              
              
              
              
              
              
              

               

              Clearly, we can implement the existing API as redirections to these methods (with suitably computed arguments) and we can also implement an API extension which deals with the cases you are interested in.

               

              These base methods could be hidden as private methods. However, I think they are actually perfectly usable as the most general level of API -- so I think they should be provided as part of the API and we should document the rest of the API as simple indirections to these base method.

               

              Now, we need to build the API back up so that we include all the original calls with implementations that can bottom out on these primitives and also add whatever other calls we might need, whether they might be convenience calls or calls necessary to distibguish cases.

               

              // Group A
              InstrumentedClass instrumentClass(Class clazz)
              InstrumentedClass instrumentClass(Class clazz, Set<String> methodNames)
              InstrumentedClass instrumentClass(Class clazz, Set<String> methodNames, boolean includeSubclasses)
              
              InstrumentedClass instrumentClass(String className, Set<String> methodNames)
              
              InstrumentedClass instrumentInterface(String interfaceName, Set<String> methodNames)
              InstrumentedClass instrumentInterface(String interfaceName, Set<String> methodNames, boolean includeSubclasses)
              
              InstrumentedClass  instrument(String className, Set<String> methodNames, boolean isInterface, boolean includeSubclasses)
              
              // Group B
              void injectOnCall(Class clazz, String methodName, String action)
              void injectOnExit(Class clazz, String methodName, String action)
              void injectOnMethod(Class clazz, String methodName, String condition, String action, String where)
              
              void injectOnMethod(Class clazz, String methodName, String condition, String action, String where, boolean includeSubclasses)
              
              void injectOnCallClass(String classname, String methodName, String action)
              void injectOnExitClass(String classname, String methodName, String action)
              void injectOnMethodClass(String classname, String methodName, String condition, String action, String where)
              
              void injectOnMethodClass(String classname, String methodName, String condition, String action, String where, boolean includeSubclasses)
              
              void injectOnCallInterface(String interfaceName, String methodName, String action)
              void injectOnExitInterface(String interfaceName, String methodName, String action)
              void injectOnMethodInterface(String interfaceName, String methodName, String condition, String action, String where)
              
              void injectOnMethodInterface(String interfaceName, String methodName, String condition, String action, String where, boolean includeSubclasses)
              
              void  injectOnMethod(String classname, String methodName, String condition, String  action, String where, boolean isInterface, boolean includeSubclasses)
              
              // Group C
              injectFault(Class clazz, String methodName, Class<? extends Throwable> fault, Object[] faultArgs)
              
              injectFaultClass(String className, String methodName, Class<? extends Throwable> fault, Object[] faultArgs)
              injectFaultInterface(String interfaceName, String methodName, Class<? extends Throwable> fault, Object[] faultArgs)
              
              injectFaultClass(String className, String methodName, Class<? extends Throwable> fault, Object[] faultArgs, boolean includeSubclasses)
              injectFaultInterface(String interfaceName, String methodName, Class<? extends Throwable> fault, Object[] faultArgs, boolean includeSubclasses)
              
              injectFault(String className, String methodName, Class<? extends Throwable> fault, Object[] faultArgs, boolean isInterface, boolean includeSubclasses)
              
              // group D
              void crashAtMethodExit(Class clazz, String methodName)
              void crashAtMethodEntry(Class clazz, String methodName)
              
              void crashAtMethodEntry(String className, String methodName) // deprecated retained for compatibility
              void crashAtMethodExit(String className, String methodName) // deprecated retained for compatibility
              void crashAtMethodEntryClass(String className, String methodName)
              void crashAtMethodExitClass(String className, String methodName)
              
              void crashAtMethodEntryInterface(String interfaceName, String methodName)
              void crashAtMethodExitInterface(String interfaceName, String methodName)
              
              void crashAtMethod(String className, String methodName, String where) // deprecated retained for compatibility
              void crashAtMethodClass(String className, String methodName, String where)
              
              void crashAtMethodInterface(String className, String methodName, String where)
              
              void crashAtMethodEntryClass(String className, String methodName, boolean includeSubclasses)
              void crashAtMethodExitClass(String className, String methodName, boolean includeSubclasses)
              void crashAtMethodEntryInterface(String interfaceName, String methodName, boolean includeSubclasses)
              void crashAtMethodExitInterface(String interfaceName, String methodName, boolean includeSubclasses)
              
              void crashAtMethodClass(String className, String methodName, String where, boolean includeSubclasses)
              void crashAtMethodClass(String className, String methodName, String where, boolean includeSubclasses)
              void crashAtMethodInterface(String interfaceName, String methodName, String where, boolean includeSubclasses)
              void crashAtMethodInterface(String interfaceName, String methodName, boolean includeSubclasses)
              
              void crashAtMethod(String className, String methodName, String where, boolean isInterface, boolean includeSubclasses)
              
              
              

               

              This API allows for you to specify more or less of the configuration elements as you want. If yo provide a Class<?> then it finesses the question of whether it is an interface or not by testing the class and calling the relevant underlying flavour with just the name if the Class<?>. The underlying methods which take a name are labelled with Class or Interface as far as possible. Older forms with the same signature which omitted the Class suffix are retained for compatibilty and need to be i) deprecated and ii) documented as translations to the corresponding method with suffix class. Eventually they all bottom out at methods whcih take String anmes only and use a flag to specify whethert te name is a class or interface. Only the lower levels allows specification as to whether to inject into subclasses (this avoids multiplying what are already a large set of alternatives).

               

              I think this is the best way to present the desired options -- you can use Vlass<?> instances if you have them but otherwise do all you want with simple names. The different flavours allow you to specify what you want at different levels of complexity. What do you think? Also, is the API here correct and consistent? Or have I messed anything up?

               

              regards,

               

               

              Andrew Dinn

              • 4. Re: Dtest library change to support INTERFACE in any Intrumentor method
                ochaloup

                Hi Andrew,

                 

                thanks a lof for your reaction and help in this. I've got your point and I will do changes based on your code change proposal.

                 

                I will let you know when it's ready.

                 

                Ondra

                • 5. Re: Dtest library change to support INTERFACE in any Intrumentor method
                  adinn

                  Hi Ondra,

                   

                  Hmm, I noticed that I omitted these convenience methods from Group A of the new API

                   

                  InstrumentedClass instrumentClass(Class clazz, boolean includeSubclasses)
                  InstrumentedClass instrumentClass(Class clazz, Set<String> methodNames, boolean includeSubclasses)
                  

                   

                  I am not sure it actually makes sense to have the first one. It is probably better only to inject into subclasses when you know that it makes sense for the method in question. If you think it might be useful then you can add it. The second one makes it easy to wrap up several injection calls into one so I think it is not a bad idea to provide it. What do yo think?

                  • 6. Re: Dtest library change to support INTERFACE in any Intrumentor method
                    ochaloup

                    Hi adinn,

                     

                    eventually I got to this thread. I was evaluating your idea once again and my feeling is that the way how rules are constructed in dtest should get more variability. I took direction of enhancing functionality of current rule builder. By my view it provides more flexibility for future.

                     

                    My proposal of changes for dtest could be found in branch: https://github.com/ochaloup/byteman/commits/dtest-instrumentor-builder

                     

                    I would be really glad if you can check it and tell me what's your position for such changes for dtest library.

                     

                    Thanks again

                     

                    As a side note: as we talked I would like to enhance the doc as we talked just I would like to discuss this subject (and maybe this one?)

                    • 7. Re: Dtest library change to support INTERFACE in any Intrumentor method
                      ochaloup

                      After some thinking I changed the code in branch a little bit. I decided to create a new builder class which would help to construct byteman rule with more context when writing a rule.

                      • 8. Re: Dtest library change to support INTERFACE in any Intrumentor method
                        adinn

                        Hi Ondra,

                         

                        The builder API is a much better idea and the implementation looks good to me. I'll pull the changes into the master repo and then prepare a 3.0.7 release from it asap (I almost have the 4.0.0-BETA3 release ready so I need to get that out first).

                         

                        Thanks very much for this excellent contribution.

                        • 9. Re: Dtest library change to support INTERFACE in any Intrumentor method
                          ochaloup

                          Thank you for the review. I hope it will be useful for others too. I plan to follow with some additions to documenation of dtest library but I'm not sure how fast it will go. It could be added to whatever future release when it will be accepted.