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

Runtime Agent API #61

Merged
merged 5 commits into from
Jan 8, 2013
Merged

Runtime Agent API #61

merged 5 commits into from
Jan 8, 2013

Conversation

marchof
Copy link
Member

@marchof marchof commented Dec 28, 2012

For special integration scenarios or experimental usages an optional API to the JaCoCo agent should be provided that can be accessed from the running JVM. This API comes as a separate bundle and can be bundled with an application if needed. The API should be designed in a way that it does not require additional dependencies e.g. on org.jacoco.core.

Support operations are:

  • Get agent version
  • Set/get session id
  • reset execution data
  • Dump (and optionally reset) execution data via configured output
  • Dump (and optionally reset) execution data locally to OutputStream

@marchof
Copy link
Member Author

marchof commented Dec 28, 2012

To allow usage with existing EMMA integrations the following API should also be supported:

com.vladium.emma.rt.RT.dumpCoverageData(File outFile, boolean merge, final boolean stopDataCollection)

@marchof
Copy link
Member Author

marchof commented Dec 29, 2012

@Godin I added a initial implementation which directly depends on agent classes. To make this work the dependencies in the API classes has to be transformed the same way than the agent packages. I wonder whether this is possible with the shade plug-in. Alternatively this could be easily done with ASM.

@Godin
Copy link
Member

Godin commented Dec 29, 2012

@marchof I don't think that this is possible with maven-shade-plugin, because those classes not in the agent module. And I wonder how to integrate into build ASM transformations? Another option - usage of reflection. BTW, why separate module? Why not in the same as agent?

@Godin
Copy link
Member

Godin commented Dec 29, 2012

Also actually we can use an MBean to access an agent without tricks with relocation of packages, since it will be registered by default ( #62 ), but unfortunately package "javax.management" not available on Android.

@marchof
Copy link
Member Author

marchof commented Dec 29, 2012

@Godin regarding ASM transformations: We might need to create our own maven-plugin that does the job as part of the build. Or simply run the utility as a Java program.

@marchof
Copy link
Member Author

marchof commented Dec 29, 2012

@Godin regarding separate module, this was my original motivation:

  1. The API is optional and is therefore not exposed by the agent itself
  2. The agent contributes classes to the application classpath. In some scenarios (e.g. osgi containers) this is not visible to user code. The API bundle could use some other mechanism to connect to the agent (like instrumented classes do today).

But for now I can live with both limitations. We can still think about different solutions if we encounter real-life scenarios where these points turn out as blockers.

My only remaining concern is package naming. We have

  • org.jacoco.agent for the bundles that provides the agent as a resource
  • org.jacoco.agent.rt for the agent implementation itself (not exposed an an API)
  • org.jacoco.agent.api for the runtime API, which is actually a strange name

@marchof
Copy link
Member Author

marchof commented Jan 7, 2013

@Godin I reworked my the implementation: Now org.jacoco.agent.rt exposes the APIs directly. Can you give this another try with your Android prototype?

@Godin
Copy link
Member

Godin commented Jan 7, 2013

@marchof Out of curiosity - why you changed your mind?

@marchof
Copy link
Member Author

marchof commented Jan 7, 2013

@Godin To keep things simple for now. We can still separate it if we run in problems in certain scenarios.

@Godin
Copy link
Member

Godin commented Jan 7, 2013

@marchof Ok, I will check this today in the evening.

@Godin
Copy link
Member

Godin commented Jan 8, 2013

@marchof Hi Marc,

Sorry for the delay - it took me quite some time to understand why this doesn't work on Android with LocalController. And here is an explanation: LocalController closes the stream only on shutdown (what not happens on Android) and there is no call of method "flush" in writeExecutionData.

One of the ways to fix this - is to invoke "flush". But in this case stream anyway remains unclosed. Can it lead to side effects?

Another option is to update "com.vladium.emma.rt.RT" to not call "dump", but instead use "getExecutionData" to write dump into specified file. This will also allow to use this compatibility layer with output mode "none" ( #63 ).

WDYT?

@marchof
Copy link
Member Author

marchof commented Jan 8, 2013

@Godin We can actually re-think the current LocalController implementation which keeps the exec file open. In e9c536c we changed it this way to fail fast when the file can't be created. But I think we can change the implementation in a way that on startup a empty file is created and for every dump we open and close the file. Looks cleaner to me anyways.

If you agree with this approach I work on this later today.

@Godin
Copy link
Member

Godin commented Jan 8, 2013

@marchof Indeed - this solution sounds good and looks cleaner. But for me - this is not an opposite (but an addition) to the modification of "com.vladium.emma.rt.RT". So I would prefer to make both modifications.

@Godin
Copy link
Member

Godin commented Jan 8, 2013

@marchof BTW The way how we work with a file seems to be related to #52

@marchof
Copy link
Member Author

marchof commented Jan 8, 2013

@Godin You're right about emma.rt.RT implementation. I'll implement it as you proposed (interpreting the 'merge' parameter as append). So we can change the LocalController in a different story.

@marchof
Copy link
Member Author

marchof commented Jan 8, 2013

@Godin Done. Can you please re-test? I'll add some more unit tests and want to merge this to continue on mbean and output=none.

@Godin
Copy link
Member

Godin commented Jan 8, 2013

@marchof Cool! Now it works as expected, so let's go for merge!

marchof added a commit that referenced this pull request Jan 8, 2013
@marchof marchof merged commit 0c1bbb2 into master Jan 8, 2013
@marchof marchof deleted the issue-61 branch January 8, 2013 17:59
@Godin
Copy link
Member

Godin commented Jan 8, 2013

@marchof Don't forget to update changelog ;)

@marchof
Copy link
Member Author

marchof commented Jan 8, 2013

Didn't I?

0c1bbb2#L40R26

@Godin
Copy link
Member

Godin commented Jan 8, 2013

Yep, indeed.

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants