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

Separate Agent MBean from Output #62

Merged
merged 2 commits into from Jan 9, 2013

Conversation

Projects
None yet
2 participants
@marchof
Member

marchof commented Jan 8, 2013

The JaCoCo agent should register an MBean separately to the output mode. This allows to use the MBean in combination with an output mode (e.g. trigger dump to file via MBean). The MBean will then have the same API as the runtime API defined in #61:

  • 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 to byte[]

The MBean is registered by default. This behavior can be modified with the new boolean agent option mbean (e.g. mbean=false).

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 29, 2012

Member

If MBean registered by default, then what to do with an existing output mode "mbean" ? Possible options - fully drop it, or it will behave as "none".

Member

Godin commented Dec 29, 2012

If MBean registered by default, then what to do with an existing output mode "mbean" ? Possible options - fully drop it, or it will behave as "none".

@Godin Godin referenced this pull request Dec 29, 2012

Merged

Runtime Agent API #61

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Dec 29, 2012

Member

Also should be noted that as mentioned in #61 package "javax.management" not available on Android.
And BTW package "java.lang.instrument" also not available on Android, which is not fatal, but leads to messages in log like:

W/ClassPathPackageInfoSource(  775): java.lang.ClassNotFoundException: org.jacoco.agent.rt.CoverageTransformer
W/ClassPathPackageInfoSource(  775):    at java.lang.Class.classForName(Native Method)
W/ClassPathPackageInfoSource(  775):    at java.lang.Class.forName(Class.java:217)
W/ClassPathPackageInfoSource(  775):    at android.test.ClassPathPackageInfoSource.createPackageInfo(ClassPathPackageInfoSource.java:88)

And I don't know how to suppress those messages without removal of classes.

Member

Godin commented Dec 29, 2012

Also should be noted that as mentioned in #61 package "javax.management" not available on Android.
And BTW package "java.lang.instrument" also not available on Android, which is not fatal, but leads to messages in log like:

W/ClassPathPackageInfoSource(  775): java.lang.ClassNotFoundException: org.jacoco.agent.rt.CoverageTransformer
W/ClassPathPackageInfoSource(  775):    at java.lang.Class.classForName(Native Method)
W/ClassPathPackageInfoSource(  775):    at java.lang.Class.forName(Class.java:217)
W/ClassPathPackageInfoSource(  775):    at android.test.ClassPathPackageInfoSource.createPackageInfo(ClassPathPackageInfoSource.java:88)

And I don't know how to suppress those messages without removal of classes.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 29, 2012

Member

I don't think mbean output is widely used, so we can drop this and clearly document this in the change log. On the other hand if enable it by default this might cause issues in some scenarios. So maybe the default should be no JMX.

Regarding missing APIs on Android the only solution is probably a different JaCoCo runtime packaging without JMX and Java agent support. As the error log is not a show stopper I wouldn't try to solve this right now.

Member

marchof commented Dec 29, 2012

I don't think mbean output is widely used, so we can drop this and clearly document this in the change log. On the other hand if enable it by default this might cause issues in some scenarios. So maybe the default should be no JMX.

Regarding missing APIs on Android the only solution is probably a different JaCoCo runtime packaging without JMX and Java agent support. As the error log is not a show stopper I wouldn't try to solve this right now.

@marchof marchof closed this Dec 29, 2012

@Godin Godin reopened this Jan 4, 2013

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 8, 2013

Member

@Godin Hi Evgeny, can you please verify that this version also works on Android? I'm asking because the current implementation has direct javax.management references from the Agent class. If this causes problems I we need to pull JMX operations into a separate class (like MBeanController was before).

Member

marchof commented Jan 8, 2013

@Godin Hi Evgeny, can you please verify that this version also works on Android? I'm asking because the current implementation has direct javax.management references from the Agent class. If this causes problems I we need to pull JMX operations into a separate class (like MBeanController was before).

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 8, 2013

Member

@Godin Thanks for reviewing, I fixed the typos. Good to merge?

Member

marchof commented Jan 8, 2013

@Godin Thanks for reviewing, I fixed the typos. Good to merge?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 8, 2013

Member

@marchof I'll test on Android tomorrow morning and will come back to you about merge.

Member

Godin commented Jan 8, 2013

@marchof I'll test on Android tomorrow morning and will come back to you about merge.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 9, 2013

Member

@marchof It works fine on Android, since the code to create MBean (i.e. which uses non-existent package) not executed by default. So we can safely merge this!

Member

Godin commented Jan 9, 2013

@marchof It works fine on Android, since the code to create MBean (i.e. which uses non-existent package) not executed by default. So we can safely merge this!

marchof added a commit that referenced this pull request Jan 9, 2013

Merge pull request #62 from jacoco/issue-62
Separate Agent MBean from Output

@marchof marchof merged commit b71f404 into master Jan 9, 2013

1 check passed

default The Travis build passed
Details

@marchof marchof deleted the issue-62 branch Jan 9, 2013

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