-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Issue #5872 - JMX DynamicMBean for jetty-slf4j-impl #5876
Conversation
Signed-off-by: Joakim Erdfelt <[email protected]>
cc8e2ab
to
37e7361
Compare
jetty-slf4j-impl/src/main/java/org/eclipse/jetty/logging/JettyLoggerFactory.java
Outdated
Show resolved
Hide resolved
{ | ||
private final JettyLoggerConfiguration configuration; | ||
private final JettyLogger rootLogger; | ||
private final ConcurrentMap<String, JettyLogger> loggerMap; | ||
private MBeanInfo mBeanInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this final
and build it in the constructor (calling a method).
This would solve the problem of getMBeanInfo()
possibly creating it twice when accessed concurrently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we create this MBeanInfo always? even for those users that don't actually use JMX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
|
||
switch (name) | ||
{ | ||
case "LoggerNames": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid loggerNames
is also a valid value, so unfortunately we need if/else
and equalsIgnoreCase()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name here and the name in MBeanInfo must match, case included.
You cannot access the MBean attribute in a case insensitive way.
Example:
MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer();
ObjectName objectName = ObjectName.getInstance("java.lang", "type", "OperatingSystem");
System.out.printf("OSVer (normal): %s%n", mbeanServer.getAttribute(objectName, "Version"));
System.out.printf("OSVer (lowercase): %s%n", mbeanServer.getAttribute(objectName, "version"));
results in ...
OSVer (normal): 5.8.0-7630-generic
Exception in thread "main" javax.management.AttributeNotFoundException: No such attribute: version
at java.management/com.sun.jmx.mbeanserver.PerInterface.getAttribute(PerInterface.java:81)
at java.management/com.sun.jmx.mbeanserver.MBeanSupport.getAttribute(MBeanSupport.java:206)
at java.management/javax.management.StandardMBean.getAttribute(StandardMBean.java:372)
at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.getAttribute(DefaultMBeanServerInterceptor.java:641)
at java.management/com.sun.jmx.mbeanserver.JmxMBeanServer.getAttribute(JmxMBeanServer.java:678)
@Override | ||
public AttributeList getAttributes(String[] attributeNames) | ||
{ | ||
Objects.requireNonNull(attributeNames, "attributeNames[]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd tolerate null
and return an empty list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is invalid per spec.
And no OpenJDK internal implementation supports null attributeNames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is invalid per spec.
Where do you see this? It's not in DynamicMBean
and I can point to implementations that tolerate null
, for example MBeanServerDelegateImpl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MBeanServerDelegateImpl.getAttributes(String[] attributes)
returns all attributes when null is passed in.
Which is nice and all, but pointless.
As the lower level code in JMX would never even call this method if the String[] attributes
is null.
MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer();
ObjectName objectName = ObjectName.getInstance("java.lang", "type", "OperatingSystem");
AttributeList attributeList = mbeanServer.getAttributes(objectName, null);
for (Attribute attr : attributeList.asList())
{
System.out.printf("Attribute[%s] = %s%n", attr.getName(), attr.getValue());
}
results in ...
Exception in thread "main" javax.management.RuntimeOperationsException: Exception occurred trying to invoke the getter on the MBean
at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.getAttributes(DefaultMBeanServerInterceptor.java:660)
at java.management/com.sun.jmx.mbeanserver.JmxMBeanServer.getAttributes(JmxMBeanServer.java:705)
at jmx.JmxAccessDemo.main(JmxAccessDemo.java:16)
Caused by: java.lang.IllegalArgumentException: Attributes cannot be null
... 8 more
@Override | ||
public AttributeList setAttributes(AttributeList attributes) | ||
{ | ||
Objects.requireNonNull(attributes, "attributes"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd tolerate null
and do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supposed to error out, as null is invalid.
All implementations within OpenJDK will throw an exception with null (different exceptions, but they all throw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MBeanServerDelegateImpl
does not throw and returns an empty AttributeList
.
jetty-slf4j-impl/src/test/java/org/eclipse/jetty/logging/JMXTest.java
Outdated
Show resolved
Hide resolved
…logging-jmx-dynamic
Signed-off-by: Joakim Erdfelt <[email protected]>
+ Fixing typo in operation names Signed-off-by: Joakim Erdfelt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart quibbles.
@Override | ||
public AttributeList getAttributes(String[] attributeNames) | ||
{ | ||
Objects.requireNonNull(attributeNames, "attributeNames[]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is invalid per spec.
Where do you see this? It's not in DynamicMBean
and I can point to implementations that tolerate null
, for example MBeanServerDelegateImpl
.
@Override | ||
public AttributeList setAttributes(AttributeList attributes) | ||
{ | ||
Objects.requireNonNull(attributes, "attributes"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MBeanServerDelegateImpl
does not throw and returns an empty AttributeList
.
{ | ||
private final JettyLoggerConfiguration configuration; | ||
private final JettyLogger rootLogger; | ||
private final ConcurrentMap<String, JettyLogger> loggerMap; | ||
private MBeanInfo mBeanInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
Convert jetty-slf4j-impl to use DynamicMBean to improve JMX support.
Signed-off-by: Joakim Erdfelt [email protected]