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 18 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.
| @@ -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.
|
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
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.
| : SignConstants.DOUBLE_MARK + portParam.getParamValue(); | ||
| ? "" | ||
| : SignConstants.DOUBLE_MARK + portParam.getParamValue(); | ||
| if (IpDomainUtil.isHasPortWithMark(instance)){ |
There was a problem hiding this comment.
I took a look at the code, and I have a question: if monitor.setInstance("unknown") is true, will this line of code execute?
Conversely, if that's not the case, I've noticed that the value of instance doesn't include port information during editing, so this line of code would never execute?
Please help confirm this issue.
There was a problem hiding this comment.
The instance is not edited directly on the frontend; instead, the host is assigned to the instance during submission, and the port parameter is appended on the backend to form the final instance. There are two scenarios:
- When the scrape type is "static," the frontend host is mandatory. The frontend assigns the host to the instance, and the backend appends the port to form the
host:portformat, which is then assigned to the instance. - When the scrape type is "sd," the frontend host is empty, resulting in an empty instance. This is passed to the backend, but the modification logic does not handle this case, leading to an error at the following line:
Map<String, String> metadata = Map.of(CommonConstants.LABEL_INSTANCE_NAME, monitor.getName(),
CommonConstants.LABEL_INSTANCE, monitor.getInstance());Therefore, we need to add a check: if this is an "sd" type monitor, set the instance to "unknown." Given the current situation, it is true that the instance cannot contain a port. Are you suggesting that we should append the port directly without checking if the instance already contains one?
There was a problem hiding this comment.
Please thoroughly test all page functions before determining whether there is an issue here. And don’t just rely on AI assistance.
|
pls check ci |
… into fix-modify-monitor-instance-bug
Removed the unnecessary check for port existence in the `instance` string, as `instance` will not contain port information in this context.
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