Fix missing historical metrics in trino-jmx #12782
Conversation
1) Some mbeans may be registered to MBeanServer after server launch, like metrics of resource groups. So that we must refresh the dynamic mbean tables regularly. 2) It should use table name instead of object name to get records from JmxHistoricalData, since on mbeans with attribute name and type, the table name is different from object name.
xmuyoo
force-pushed
the
fix-missing-history-metircs-trino-jmx
branch
from
1c810c8
to
df9931d
Compare
xmuyoo
requested review from
aakashnand
and removed request for
Praveen2112 and
aakashnand
| public synchronized List<List<Object>> getRows(String tableName, List<Integer> selectedColumns) | ||
| { | ||
| String lowerCaseObjectName = objectName.toLowerCase(Locale.ENGLISH); | ||
| if (!tableData.containsKey(lowerCaseObjectName)) { | ||
| String lowerCaseTableName = tableName.toLowerCase(Locale.ENGLISH); | ||
| if (!tableData.containsKey(lowerCaseTableName)) { | ||
| return ImmutableList.of(); | ||
| } | ||
| return projectRows(tableData.get(lowerCaseObjectName), selectedColumns); | ||
| return projectRows(tableData.get(lowerCaseTableName), selectedColumns); | ||
| } |
There was a problem hiding this comment.
Since we maintain the information for each JMXObject - I think objectName is a correct terminology here
There was a problem hiding this comment.
Maybe we need to maintain consistency with addRow
|
|
||
| for (String tableName : tables) { | ||
| tableData.put(tableName, EvictingQueue.create(maxEntries)); | ||
| } | ||
|
|
||
| MBeanServerNotificationFilter filter = new MBeanServerNotificationFilter(); | ||
| filter.enableAllObjectNames(); |
There was a problem hiding this comment.
Can we specify object name as filters ?
| @@ -91,4 +114,42 @@ private List<List<Object>> projectRows(Collection<List<Object>> rows, List<Integ | |||
| } | |||
| return result.build(); | |||
| } | |||
|
|
|||
| public class AddDynamicTableListener | |||
There was a problem hiding this comment.
Can we unify this with mbeanServer#queryNames - so we could add an entry to the Map if it is matched and we could rewrite getTables to tableData#keySet.
| @@ -54,16 +56,9 @@ public JmxPeriodicSampler( | |||
| this.jmxRecordSetProvider = requireNonNull(jmxRecordSetProvider, "jmxRecordSetProvider is null"); | |||
| requireNonNull(jmxConfig, "jmxConfig is null"); | |||
| this.period = jmxConfig.getDumpPeriod().toMillis(); | |||
| this.jmxMetadata = jmxMetadata; | |||
There was a problem hiding this comment.
Can we add require non-null check here ?
| rows = tableHandle.getObjectNames().stream() | ||
| .flatMap(objectName -> jmxHistoricalData.getRows(objectName, selectedColumns).stream()) | ||
| .collect(toImmutableList()); | ||
| rows = ImmutableList.copyOf(jmxHistoricalData.getRows(tableHandle.getTableName().getTableName(), selectedColumns)); |
There was a problem hiding this comment.
I think - we don't need this change. A JMXTableName could be a regex and it could match with a bunch of JMXObjects to we need to do a flatmapping and fetch the information for all the objects which matches the query pattern.
| this.tableNames = tableNames; | ||
| this.tableData = tableData; | ||
| this.tables = tables; |
| if (tableData.containsKey(lowerCaseTableName)) { | ||
| tableData.get(lowerCaseTableName).add(row); | ||
| } |
There was a problem hiding this comment.
Can we revert this ? We should throw an error if the table doesn't exist
| assertEquals(jmxHistoricalData.getRows(TABLE_NAME, columns), ImmutableList.of()); | ||
|
|
||
| StandardMBean newMBean = new StandardMBean(new ArrayList<>(), List.class); | ||
| mbeanServer.registerMBean(newMBean, ObjectName.getInstance(ON_FLY_TABLE_NAME)); |
There was a problem hiding this comment.
Instead of injecting a new Bean can we try accessing the InternalResourceGroup - mentioned in the actual issue ?
|
👋 @xmuyoo - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that. Cc @Praveen2112 We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. |
Description
The historical metrics of resource groups in
jmx.historyare missing.Here's the reasons:
Some mbeans may be registered to MBeanServer after server launch,
like metrics of resource groups. So that we must refresh the dynamic
mbean tables regularly.
It should use table name instead of object name to get records
from JmxHistoricalData, since on mbeans with attribute name and type,
the table name is different from object name.
See: #12781
Related issues, pull requests, and links
Documentation
( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
( ) No release notes entries required.
( ) Release notes entries required with the following suggested text: