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

[BTRACE-45] Print multiple aggregations in one printAggregation() command #35

Closed
jbachorik opened this issue Aug 28, 2014 · 8 comments
Closed

Comments

@jbachorik
Copy link
Collaborator

[reporter="joachimhskeie", created="Sat, 17 Jul 2010 23:09:55 +0200", resolved="Mon, 23 Aug 2010 22:34:50 +0200"]

Often when reporting on Average Execution times for classes, I would like to be able to report on both the average execution time and the number of executions that average is calculated from via the same printAggregation method.

I am attaching a diff file that adds a "CountingAverage" aggregation type, extending from the Average aggregation type. I am also modifying the GridDataCommand to support the new CountingAverageData class.

I am not quite happy with the following, although I dont have any good suggestions for improvements without refactoring the methods signatures:

  • the instanceof in the GridDataCommand makes for (potentially) bloated code if new aggregation data types are added
  • I would like for the CountingAverageData to be able to use a custom StringFormat to format its output, but the API available via BTraceUtils does not allow information about this to be set.

Another approach to this issue would be to add a method printAggregation(String stringFormat, Aggregation ... aggregationArray) that would combine the results from multiple aggregations for output, assuming they use the same AggregationKey. This might be the better option in this (and other) case(s). I could have a look into a possible solution to this if you would rather see this type of solution.

@jbachorik jbachorik added this to the release-1.2 milestone Aug 28, 2014
@jbachorik
Copy link
Collaborator Author

[author="joachimhskeie", created="Wed, 4 Aug 2010 15:42:10 +0200"]

I am unable to make the BTrace compiler accept the varargs parameters like:

printAggregation(String stringFormat, Aggregation ... aggregationArray)

I am able to make this work with a single Aggregation, but with multiple aggregations the BTrace Compiler states that only methods for BTraceUtils might be used.

Is this a shortcoming of the BTrace Compiler class ?

@jbachorik
Copy link
Collaborator Author

[author="joachimhskeie", created="Thu, 5 Aug 2010 22:37:44 +0200"]

I found the culprit in the VerifierVisitor class. I added a bit of code that makes the compiler compile the class when the method in BTraceUtils is a varargs method. However, this would need some type checking.

Actually, looking at the code, I would assume it safer if all parameter types were checked, and not just a check to see if the method invoked has the same amount of parameters

@jbachorik
Copy link
Collaborator Author

[author="joachimhskeie", created="Thu, 5 Aug 2010 23:37:34 +0200"]

I was unable to the the varargs method to work at all (the JVM translates the varargs arguments to an Array, which causes the Verifier to think that the BTrace Script is creating a new Array.

I have instead opted to implement this functionality using the Collections that were available in BTraceUtils already.

I think this is a better option than the CountingAverage approach as this makes it possible to combine any Aggregation, as long as it has the same AggregationKey.

I am attaching a diff new diff file which implements this functionality as well.

I am using the Aggregation in a script as follows:

    @OnTimer(7500)
    public static void printAverage() {
        Deque aggregationDeque = newDeque();
        addLast(aggregationDeque, AVERAGE);
        addLast(aggregationDeque, CALLS_PER_INTERVAL);
    <span class="code-object">String</span> execStringFormat = strcat(<span class="code-quote">"[AverageExecTime;"</span>, property(<span class="code-quote">"btrace.agentname"</span>));
    Aggregations.printAggregation(<span class="code-quote">"", strcat(execStringFormat, "</span>;%1$s;%2$s;%3$d;%4$s;%5$s;%6$s;Custom]"), aggregationDeque);
    
    truncateAggregation(AVERAGE, 0);
    truncateAggregation(CALLS_PER_INTERVAL, 0);
}   

I'll leave it up to you to decide which option you like best, but I quite like the last solution best.

@jbachorik
Copy link
Collaborator Author

[author="jbachorik", created="Mon, 9 Aug 2010 11:01:15 +0200"]

The enhanced version of printAggregation() method sounds good.

Go ahead and push the changes. Please, make sure all the newly added public methods are properly documented and don't forget to add the @SInCE tag (for version 1.2)

@jbachorik
Copy link
Collaborator Author

[author="jbachorik", created="Fri, 13 Aug 2010 09:54:35 +0200"]

Will you be able to push the changes soon? If not I might apply the changes for you.
I want to start preparations for 1.2 release and I need to have all issues targeted for 1.2 resolved.

@jbachorik
Copy link
Collaborator Author

[author="joachimhskeie", created="Fri, 13 Aug 2010 10:30:45 +0200"]

I will push the changes over the weekend.

@jbachorik
Copy link
Collaborator Author

[author="joachimhskeie", created="Mon, 23 Aug 2010 22:33:54 +0200"]

Sorry for the late push! I have pushed the changes now. I chose not to push the CountingAverage Aggregation, as its probably redundant with the ability to combine aggregations into a single print.

@jbachorik
Copy link
Collaborator Author

[author="joachimhskeie", created="Mon, 23 Aug 2010 22:34:27 +0200"]

Changing Fix Version to 1.2 as per comments from JB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant