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

Conversation

Projects
None yet
2 participants
@marchof
Member

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 28, 2012

Member

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)
Member

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 29, 2012

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 29, 2012

Member

@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?

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 29, 2012

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 29, 2012

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 29, 2012

Member

@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
Member

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 7, 2013

Member

@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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 7, 2013

Member

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

Member

Godin commented Jan 7, 2013

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 7, 2013

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 7, 2013

Member

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

Member

Godin commented Jan 7, 2013

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

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 8, 2013

Member

@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?

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 8, 2013

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 8, 2013

Member

@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.

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 8, 2013

Member

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

Member

Godin commented Jan 8, 2013

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 8, 2013

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 8, 2013

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 8, 2013

Member

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

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 Jan 8, 2013

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 8, 2013

Member

@marchof Don't forget to update changelog ;)

Member

Godin commented Jan 8, 2013

@marchof Don't forget to update changelog ;)

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof
Member

marchof commented Jan 8, 2013

Didn't I?

0c1bbb2#L40R26

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 8, 2013

Member

Yep, indeed.

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.