fix(manager): set instance to 'unknow' for dynamic monitors on modification#4101
fix(manager): set instance to 'unknow' for dynamic monitors on modification#4101pentium100 wants to merge 11 commits intoapache:masterfrom
Conversation
…cation
In the `modifyMonitor` method, the `instance` field for dynamically discovered monitors (where `isStatic` is false) was not being explicitly set to "unknow". This could lead to incorrect instance values or unexpected behavior when updating service discovery monitors.
This commit aligns the behavior of `modifyMonitor` with `addMonitor` by explicitly setting `monitor.setInstance("unknow")` for non-static scrape types before processing the instance and port fields.
…n modification" This reverts commit 91f4f9f.
…cation
In the `modifyMonitor` method, the `instance` field for dynamically discovered monitors (where `isStatic` is false) was not being explicitly set to "unknow". This could lead to incorrect instance values or unexpected behavior when updating service discovery monitors.
This commit aligns the behavior of `modifyMonitor` with `addMonitor` by explicitly setting `monitor.setInstance("unknow")` for non-static scrape types before processing the instance and port fields.
There was a problem hiding this comment.
Pull request overview
Fixes an exception when modifying service-discovery (non-static scrape) monitors by ensuring monitor.instance is set to a non-null placeholder before building job metadata (aligning modifyMonitor behavior with addMonitor). This addresses issue #4100 where Map.of(...) would throw due to a null instance.
Changes:
- In
modifyMonitor, computeisStaticearlier and setmonitor.instanceto a placeholder value for non-static scrapes. - Prevent null
instancefrom reachingMap.of(...)during job metadata construction for service discovery monitors.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| boolean isStatic = CommonConstants.SCRAPE_STATIC.equals(monitor.getScrape()) | ||
| || !StringUtils.hasText(monitor.getScrape()); | ||
| if (!isStatic) { | ||
| monitor.setInstance("unknow"); |
There was a problem hiding this comment.
The placeholder instance string is spelled "unknow". If this value is user-facing or persisted, consider switching to the correctly spelled "unknown" (ideally via a shared constant) and handling any backward-compatibility/migration needed, since this method now introduces an additional persisted usage of the typo.
| monitor.setInstance("unknow"); | |
| monitor.setInstance("unknown"); |
| @@ -396,8 +402,6 @@ public void modifyMonitor(Monitor monitor, List<Param> params, String collector, | |||
| } | |||
| monitor.setInstance(instance); | |||
There was a problem hiding this comment.
modifyMonitor appends portWithMark whenever instance is non-null, even if instance already includes a ":port" suffix. This can produce duplicated ports (e.g., host:9200:9200) for API clients/imports that send monitor.instance with port included. Align this logic with addMonitor by checking IpDomainUtil.isHasPortWithMark(instance) before concatenating the port.
...beat-manager/src/main/java/org/apache/hertzbeat/manager/service/impl/MonitorServiceImpl.java
Outdated
Show resolved
Hide resolved
|
hi @pentium100 pls see the copilot review comment. |
When the monitor type is service discovery and the instance is not empty, the instance value should not be overwritten. This commit fixes this issue. Additionally, it corrects the spelling of 'unknow' to 'unknown' and prevents appending the port if the instance already contains it.
… into fix-modify-monitor-instance-bug
...beat-manager/src/main/java/org/apache/hertzbeat/manager/service/impl/MonitorServiceImpl.java
Outdated
Show resolved
Hide resolved
Duansg
left a comment
Duansg
left a comment
There was a problem hiding this comment.
In addition, please update the regression test cases for this change.
| if (Objects.nonNull(instance)) { | ||
| ? "" | ||
| : SignConstants.DOUBLE_MARK + portParam.getParamValue(); | ||
| if (Objects.nonNull(instance) && !IpDomainUtil.isHasPortWithMark(instance)) { |
There was a problem hiding this comment.
I have a question here: if this includes port-skipping concatenation, will changes made by the user to the port not take effect?
What's changed?
fix(manager): set instance to 'unknow' for dynamic monitors on modification
In the
modifyMonitormethod, theinstancefield for dynamically discovered monitors (whereisStaticis false) was not being explicitly set to "unknow". This could lead to incorrect instance values or unexpected behavior when updating service discovery monitors.This commit aligns the behavior of
modifyMonitorwithaddMonitorby explicitly settingmonitor.setInstance("unknow")for non-static scrape types before processing the instance and port fields.refer to #4100
Checklist
Add or update API