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

Default JmxAutoConfiguration changes JConsole hierarchy for multi-property @ManagedResource object names #29953

Closed
rschuetz opened this issue Feb 22, 2022 · 3 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@rschuetz
Copy link

rschuetz commented Feb 22, 2022

Affects: Spring Boot 2.2.11 (but most likely the current version as well, the code is the same)

The ParentAwareNamingStrategy bean created by JmxAutoConfiguration changes the original order of key properties of the ObjectName unconditionally, as it decomposes the original name in getObjectName() and recreates it to add, if configured, "identity" and "context" fields, by using a Hashtable to temporarily store the key properties.

An object name defined with @ManagedResource like "ABC:type=something,name1=def,name2=ghi" with more than one property except "type" could therefore change to "ABC:type=something,name2=ghi,name1=def".

While two ObjectNames with the same key properties in different order are semantically equal and the canonical name stays the same, this change may not affect regular calls through the JMX API, but it at least affects JConsole, as JConsole relies on the property order to build the MBean hierarchy in the "MBeans" tab. Other tools that for whatever reason rely on the same order as defined in the code might be affected as well.

For the case above, a hierarchy ABC > def > ghi in JConsole might change to ABC > ghi > def, which at least affects things like documentation about where to find specific operations or attributes.

It's potentially not easily possible to keep the order intact if "identity" and "context" fields would need to be added, but it might be already helpful, if ParentAwareNamingStrategy would simply use the original name as-is, if these fields don't need to be added, similar to MBeanExporter#registerManagedResource(Object), that modifies the name only if changes are needed.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 22, 2022
@snicoll snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 23, 2022
@snicoll snicoll added this to the 2.5.x milestone Feb 23, 2022
@snicoll
Copy link
Member

snicoll commented Feb 23, 2022

Thanks for the report.

It's potentially not easily possible to keep the order intact if "identity" and "context" fields would need to be added

I don't see how that's possible indeed considering that ObjectName itself uses an HashTable. However, you are right that we should only change the ObjectName if that is necessary.

@wilkinsona wilkinsona self-assigned this Feb 23, 2022
@wilkinsona
Copy link
Member

I think it may be possible to preserve the order with something like this:

private ObjectName appendToObjectName(ObjectName name, String key, String value) throws MalformedObjectNameException {
    return ObjectNameManager.getInstance(name.getDomain() + ":" + name.getKeyPropertyListString() + "," + key + "=" + value);
}

However, I don't think we should take this approach as it would not be consistent with Framework's JmxUtils.appendIdentityToObjectName. We can, however, leave the order unchanged when neither identity nor context is being added.

@rschuetz If you would like the property ordering to be preserved when the identity or context attribute is being added, please open a Spring Framework issue proposing a change to JmxUtils.appendIdentityToObjectName and comment here with a link to it. If the Framework team agree with preserving the order we can then adapt accordingly in Spring Boot.

@rschuetz
Copy link
Author

@wilkinsona: Thanks a lot for the quick resolution. The current approach is fine for me for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants