-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix metrics hints builder to avoid wrong container metadata usage when port is not exposed #18979
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
1 similar comment
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
💚 Build SucceededExpand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
|
1d6c0c1
to
7083347
Compare
Pinging @elastic/integrations-platforms (Team:Platforms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @vjsamuel, thanks for this fix.
As you mention, this PR includes two things, one fix to avoid mapping incorrect ports, and an improvement on how beats generate autodiscover events for pods. Could you please separate the change in two PRs?
I think that the enhancement may require further discussion and I wouldn't like to block one thing with the other.
Changelogs will be needed.
bd11535
to
5d1e81c
Compare
I think we can go on with this change. @vjsamuel could you take a look to the merge conflict? |
5d1e81c
to
e0a6431
Compare
jenkins, run the tests please |
ee02af1
to
08baa61
Compare
jenkins, run the tests please |
…n port is not exposed
08baa61
to
1a5eac8
Compare
jenkins, run the tests please |
…n port is not exposed (elastic#18979) (cherry picked from commit 2f7b501)
#18979 introduced a pod level event which is generated after all container events. The ordering is wrong in that pod events are sent last which would generate a valid event similar to container events. The ordering needs to be pod first and container events next so that pod events dont override valid container events. One other issue was that the pod level hint generates a single config with all hosts and it wont get over written by container hints causing more than one config to be spun up for the same hint (one with a container meta and one without).
elastic#18979 introduced a pod level event which is generated after all container events. The ordering is wrong in that pod events are sent last which would generate a valid event similar to container events. The ordering needs to be pod first and container events next so that pod events dont override valid container events. One other issue was that the pod level hint generates a single config with all hosts and it wont get over written by container hints causing more than one config to be spun up for the same hint (one with a container meta and one without). (cherry picked from commit 58edbb4)
#18979 introduced a pod level event which is generated after all container events. The ordering is wrong in that pod events are sent last which would generate a valid event similar to container events. The ordering needs to be pod first and container events next so that pod events dont override valid container events. One other issue was that the pod level hint generates a single config with all hosts and it wont get over written by container hints causing more than one config to be spun up for the same hint (one with a container meta and one without). (cherry picked from commit 58edbb4) Co-authored-by: Vijay Samuel <vjsamuel@ebay.com>
…n port is not exposed (elastic#18979)
elastic#18979 introduced a pod level event which is generated after all container events. The ordering is wrong in that pod events are sent last which would generate a valid event similar to container events. The ordering needs to be pod first and container events next so that pod events dont override valid container events. One other issue was that the pod level hint generates a single config with all hosts and it wont get over written by container hints causing more than one config to be spun up for the same hint (one with a container meta and one without).
Type of PR
What does this PR do?
I have enhanced the emitEvent to generate a pod level event with no port. The hints builder checks to see if there is a port or not. If not then it uses the pod level metadata to monitor
data.host:<port>
annotations. This ensures that we dont use the wrong container metadata to start up a metricbeat module or do it multiple times as well.Why is it important?
The way that things exist today is that when ports arent exposed, the container meta is non-deterministic
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs
Fixes: #12011