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 33 commits into
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.
… 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.
|
|
Duansg
left a comment
Duansg
left a comment
There was a problem hiding this comment.
@pentium100 Hi, Please continue reviewing the code. If you encounter any issues, please refer to the Code Style and Quality Guide first.
* Added logic in `MonitorServiceImpl.java` to check if the `instance` string contains a port, and if so, strips the port out to only retain the IP or domain.
… into fix-modify-monitor-instance-bug
…r build The Docker build was failing because /var/run/sshd already exists in the base image. Using mkdir -p allows the command to succeed if the directory already exists, preventing exit code 1 errors. Fixes apache#4101
zqr10159
left a comment
zqr10159
left a comment
There was a problem hiding this comment.
The null-instance fix for service-discovery monitors is directionally correct, but the new port-removal path corrupts valid IPv6 instances. IpDomainUtil.isHasPortWithMark("2001:db8::1") treats the final numeric segment as a port, so lines 394-395 truncate an unbracketed IPv6 address to 2001:db8: even when no port parameter is supplied. Please use an IPv6-safe host/port representation or parser and add focused modify-monitor cases for IPv4/domain hosts plus bracketed and unbracketed IPv6, both with and without ports. The large unrelated indentation churn should also be removed before merge.
zqr10159
left a comment
zqr10159
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest-master merge. The previous IPv6 truncation and formatting churn are gone; dynamic monitor identity is non-null, and explicit port replacement/removal is covered for domains, IPv4, raw IPv6, and bracketed IPv6. The complete MonitorServiceTest suite passes locally (46 tests) with Checkstyle.
What's changed?
Normalize monitor instances when adding or modifying monitors.
unknown, preventing anull instance from reaching immutable metadata construction
current port parameter
host:port; a configured port producesthe unambiguous bracketed form (
[2001:db8::1]:443)masterand the earlier formatting-onlychurn has been removed
Regression proof
The latest-master baseline reproduced the service-discovery failure with
expected: <unknown> but was: <memory>. The corrected implementation passes:MonitorServiceTesttestsIPv6, port addition, replacement, and removal
git diff apache/master --checkOCR delegation resolved the Java correctness, boundary, performance, and thread
safety rules. The test diff was manually reviewed because the deterministic
selector excludes test paths; no credible high or medium finding remains.
AI assistance: used for conflict resolution, regression coverage, and test
iteration.
Human validation: reproduced the old failure and ran the complete
MonitorServiceTestsuite on Java 25.Risk notes: an unbracketed IPv6 literal is always treated as a host; ports are
represented with brackets to avoid an ambiguous final numeric segment.