Skip to content
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

Feature: Support discovery monitoring instance through http_sd #1791

Closed
wants to merge 4 commits into from

Conversation

Calvin979
Copy link
Contributor

@Calvin979 Calvin979 commented Apr 19, 2024

What's changed?

Corresponding issue:
#1625

Service discovery can be available for all collectors now and I provide http_sd as a deafult strategy.
There's something different between one time task and cyclic task while fetching instance info:

  1. one time task
    image
  2. cyclic task
    image

How to add service discovery for collector?

  1. implements CommonProtocol interface
    image
  2. add param field like http_sd in yml
    image
  - field: http_sd
    name:
      zh-CN: http_sd
      en-US: http_sd
    type: text
    placeholder: Enter http_sd url
    required: false
    hide: true
  1. override method getSupportProtocolClass and point out which protocol is used for current collector
    image

Test result
01测试连接成功
02监控详情

What's more
the priority of sd is higher than host, which means it will use sd first if you config host and http_sd simultaneously.

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

@Calvin979
Copy link
Contributor Author

Calvin979 commented Apr 19, 2024

I haven't add sd for any collectors, it needs to be discussed which collector should be supported for sd

@tomsun28 tomsun28 self-requested a review April 20, 2024 03:39
@tomsun28
Copy link
Contributor

👍👍 This design implementation is eye-catching and is a good idea. I don't seem to find the part that automatically discovers and creates multiple instances through the information returned by the http_sd interface. For example, http_sd returns ip1-port1 ip2-port2 ip3-port3. We create three new monitoring instances based on these three automatic discoveries.

@Calvin979
Copy link
Contributor Author

Calvin979 commented Apr 20, 2024

@tomsun28
I'm not sure whether you mean we need to create monitor task automatically. If so, the process is different.
Monitor task must be created and managed by Manager, then collector should collect sd data as a cyclic task like this:
image
In this case I think batch create task would be better, it will change a lot of code and design.

What I coded in this pr is update host and port automatically.
image
It will be helpful for monitoring services that are supported for automatic failover like Redis sentinel.

@tomsun28
Copy link
Contributor

hi yes, the first design like this as same as your.

image

image

Also i think this pr design is great, provides another way of thinking. how about combine the two. Our initial goal is to support automatic maintenance of batch monitoring tasks through the data returned by http_sd.

@Calvin979
Copy link
Contributor Author

@tomsun28
Thanks for your design. It is a more clear design than my design. Should I modify code in this pr directly?🤣🤣

@tomsun28
Copy link
Contributor

👍👍 okay okay. It is not urgent, depends on your time. Recommend to release an first apache version first and after then consider integrate this big feature.

@Calvin979
Copy link
Contributor Author

👍👍 okay okay. It is not urgent, depends on your time. Recommend to release an first apache version first and after then consider integrate this big feature.

OK, I will close this pr. And I will open another pr after first apache version released and I finished this new feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants