Add support for customized monitoring API #22605

newly12 · 2020-11-17T03:28:59Z

What does this PR do?

allow customized monitoring api to be added

Why is it important?

allow customized monitoring api to be added

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~- [ ] I have made corresponding changes to the documentation~~
~~- [ ] I have made corresponding change to the default configuration files~~
~~- [ ] I have added tests that prove my fix is effective or that my feature works~~
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

N/A

How to test this PR locally

N/A

Use cases

Leverage beats' http server, same port to be used, to expose any other info that user likes to do in a beat implementation or customization.

newly12 · 2020-11-17T04:29:44Z

test failure does not seem to be related to this PR..

elasticmachine · 2020-11-17T07:10:48Z

Pinging @elastic/integrations-services (Team:Services)

elasticmachine · 2020-11-17T10:31:03Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: urso commented: jenkins run the tests please
Start Time: 2020-12-19T19:11:28.449+0000
Duration: 55 min 27 sec

Test stats 🧪

Test	Results
Failed	0
Passed	17504
Skipped	1383
Total	18887

Steps errors

Expand to view the steps failures

`Terraform Apply on x-pack/metricbeat/module/aws`

Took 0 min 15 sec . View more details on here
Description: terraform apply -auto-approve

`Terraform Apply on x-pack/metricbeat/module/aws`

Took 0 min 16 sec . View more details on here
Description: terraform apply -auto-approve

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	17504
Skipped	1383
Total	18887

sayden · 2020-11-18T10:46:41Z

Please, can you explain why is this necessary for Beats? In different words so maybe I'm more helpful: Why Beats needs this feature? Why Beats will need a customized monitoring API, who can benefit from it and how that user will gain benefit from it?

Also, how a user is expected to use it?

newly12 · 2020-11-18T13:10:34Z

Hi @sayden ,

Sorry for filing the PR directly, maybe I should start a thread to discuss further on this first, let me know.

I currently have one use case that I am trying to add some additional info in filebeat such as how many events it sends out having specific labels that we care about, though libbeat/monitoring package provides API to register any metrics given a json path, it seems not quite suit for the labels case, IMHO in the ingestion phase, in order to have metricbeat to convert sub path(s) to label(s), sounds like a bit more tricky and the paths probably need to be explicitly given in processor config if I am not wrong, and they could change from server to server. That being said, what I want to achieve is exposing these additional metrics in prometheus format so metricbeat processes them naturally.

This might be very specific edge case that rare user might face, but this change allows user to do this without forking and maintaining own version of beats and saves potential effort of solving conflicts if any.

This change in beats appears to be the easiest approach that I can think of, after this is added, in our implementation I could initialize prometheus registry and hook its handler in filebeat's http server before execute BeatsRootCmd, and afterwards in any other beat component implementation(i.e. input, processor, output) have needed metrics registered.

Please let me know if any comment or suggestion.

vjsamuel · 2020-11-25T07:06:01Z

@sayden, we are trying to come up with a processor that can:

read log lines and come up with some basic parsing/manipulations and do counting
expose the counted data as a prometheus endpoint

This would allow us to use metricbeat to send data to the backend as metrics which has better query performance.

This is a pattern that is widely adopted by other agents like grafana cloud agent, otel collector. For us to be able to accomplish such a use case, we would need to bind a metrics endpoint into the http server. How would you recommend us moving forward on this? Do you think that this PR would make sense in that aspect or do you think that the processor should start up the http server itself on a different port?

Thanks in advance.

cc: @exekias what do you think>

exekias · 2020-11-26T10:07:50Z

Thanks for the explanations! I chatted a little bit about this with @urso. This API will probably be refactored at some point, so take into account that may affect you, but for now, this change sounds good to me

newly12 · 2020-11-30T08:51:57Z

@exekias thanks for your comment.

@sayden could you please comment and advise how should we move forward on this? Thanks in advance.

urso · 2020-12-10T15:19:43Z

libbeat/api/routes.go

+
+	for api, h := range handlerFuncMap {
+		mux.HandleFunc(api, h)
+	}


It would be nice if we can dynamically add/remove routes. E.g. when starting/removing sub-systems. The problem with a "global" like this one is initialization order. You can not tell when NewWithDefaultRoutes is called. This requires anyone to call AddHandlerFunc from within a init function. The response for routes can be changed (via globals), but the presence/behavior can hardly be configured.

Thanks @urso ! it makes sense for adding/removing routes. I had an implementation for this, please have a check.

There is still a "global" routes in the implementation that shares its http.ServeMux with the server initialized in NewWithDefaultRoutes. The reason for this is that I think there needs to be a "global" thing, either routes or a "global" api.Server, in order to allow register/deregister routes, and though there are other ways to initialize api.Server such as its New function, but I don't see it's being used except NewWithDefaultRoutes.

urso · 2020-12-10T15:20:37Z

libbeat/api/routes.go

+
+// AddHandlerFunc provides interface to add customized handlerFunc
+func AddHandlerFunc(api string, h handlerFunc) {
+	handlerFuncMap[api] = h


We should error/panic if the api is already in use. A new handlerFunc should not be allowed to replace an older handlerFunc.

This is now considered in the new implementation.

reverted to previous implementation and added check in this function..

urso

All in all I'm not sure about this change. How do you plan to use these hooks?

Although it is nice to be able to deregister handlers, I wonder if the introduction of this + still having a global routes table with absolute URL prefixes is a good "solution". The "global" mutex might be a problem if you have more than one client accessing the API.

(Long term) If possible I'd like to pass some "context" to sub-systems in Beats via an interface that manipulates the "local" routing path. Something like this (kind similar to gorilla mux, but gorilla does not support removal):

type Router interface {
  Handle(path string, handler http.Handler) (Route, error)
  HandleFunc(path string, fn http.HandleFunc) (Route, error)
}

type Route interface {
  Close() error // remove the route
  SubRoute() Router // allow more handlers to be registered based on current path as prefix, sub handlers get removed when this route is closed.
}

The "root-router" would be passed to the server as parameter. Yet without actual use-cases in mind I'd rather not introduce that much code.

urso · 2020-12-15T18:17:20Z

libbeat/api/routes.go

+		}
+	}
+	if log == nil {
+		log = logp.NewLogger("")


The logger needs a name. The names are used as selectors when enabling debug logging. How about api?

urso · 2020-12-15T18:23:25Z

libbeat/api/routes.go

+		log = logp.NewLogger("")
+	}
+	if routes.log == nil {
+		routes.log = log


hm, to me this is a red flag. Using structured logging we don't know which logger context is the correct one. Either we have a singleton pattern or not. But having a global routes + a local server instance is kind of contradictory to me.

In order to make this PR not to big I understand that we still need globals. Although I would like to see something passed as contextual parameter to other subsystems...

Edit: Actually checking the Routes implementation I don't think we need the logger.

urso · 2020-12-15T18:28:17Z

libbeat/api/routes.go

+	}
+	if _, exist := d.routes[api]; exist {
+		err := fmt.Errorf("route %s is already in use", api)
+		d.log.Error(err.Error())


We return errors from different places, but this is the only place we log. How about removing this line, then we don't need the logger here.

urso · 2020-12-15T19:15:06Z

libbeat/api/routes.go

+	d.mux.Lock()
+	defer d.mux.Unlock()
+	if !strings.HasPrefix(api, "/") {
+		return fmt.Errorf("route should starts with /")


I think it is quite unfortunate that we need to use absolute addresses when registering. This seems to be a byproduct of having Routes global.

At some point we need to think about some proper API for sub-systems to register/deregister routes. Kind of similar to gorilla/mux SubRoutes.

urso · 2020-12-15T19:16:18Z

libbeat/api/routes.go

+
+func (d *Routes) handle(w http.ResponseWriter, r *http.Request) {
+	d.mux.RLock()
+	defer d.mux.RUnlock()


these locks are "heavy". Although we have an http server, this look will allow to have only one active connection at a time.

…est case" This reverts commit 7d6142a75d06800cf4a6f03b93cbeacd6e10c467.

urso · 2020-12-17T19:19:52Z

/test

urso · 2020-12-19T19:11:02Z

jenkins run the tests please

newly12 · 2020-12-22T00:55:28Z

Thanks for merging this PR!

(cherry picked from commit 82c11d4)

ycombinator · 2020-12-29T19:14:51Z

libbeat/api/routes.go

@@ -38,6 +40,10 @@ func NewWithDefaultRoutes(log *logp.Logger, config *common.Config, ns lookupFunc
 	mux.HandleFunc("/state", makeAPIHandler(ns("state")))
 	mux.HandleFunc("/stats", makeAPIHandler(ns("stats")))
 	mux.HandleFunc("/dataset", makeAPIHandler(ns("dataset")))


I know this PR is already merged but I just noticed this while reviewing the backport PR. Should these three API routes be added via AddHandlerFunc too? That would ensure that someone doesn't try to override these routes when registering a custom one.

Thanks for the comments.

As mentioned in this reply, the usage of AddHandlerFunc would be registering other routes in a module's "init" phase, that makes customized routes to be shown up before "default" routes, and the reason "default" routes can not be done in "init" phase is the handler functions relies on ns lookupFunc which is passed only in NewWithDefaultRoutes.

I think what can be done is

call AddHandlerFunc for default routes as well so at least errors can be logged when there are conflicts.

or move default routes registration to init phase and hardcode the ns to be monitoring.GetNamespace and change the interface of NewWithDefaultRoutes .

Any thoughts?

…23279) Co-authored-by: Peter Deng <[email protected]>

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 17, 2020

newly12 changed the title ~~allow customized monitoring api to be added~~ Add support for customized monitoring API Nov 17, 2020

andresrc added the Team:Services (Deprecated) Label for the former Integrations-Services team label Nov 17, 2020

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 17, 2020

andresrc assigned sayden Nov 17, 2020

urso reviewed Dec 10, 2020

View reviewed changes

newly12 force-pushed the customzied_api branch from 8b08095 to 7d6142a Compare December 11, 2020 06:20

urso reviewed Dec 15, 2020

View reviewed changes

newly12 added 5 commits December 16, 2020 12:51

allow customized monitoring api to be added

b358184

update CHANGELOG.next.asciidoc

bfbed50

support register/deregister routes dynamically and refactor test case

b1d3bc9

Revert "support register/deregister routes dynamically and refactor t…

ebc290c

…est case" This reverts commit 7d6142a75d06800cf4a6f03b93cbeacd6e10c467.

check handler func existence before adding it

8222f53

newly12 force-pushed the customzied_api branch from 2a44bea to 8222f53 Compare December 16, 2020 04:56

Merge branch 'master' into customzied_api

340a67a

urso added the needs_backport PR is waiting to be backported to other branches. label Dec 21, 2020

urso approved these changes Dec 21, 2020

View reviewed changes

urso merged commit 82c11d4 into elastic:master Dec 21, 2020

newly12 deleted the customzied_api branch December 22, 2020 00:55

urso mentioned this pull request Dec 24, 2020

Cherry-pick #22605 to 7.x: Add support for customized monitoring API #23279

Merged

3 tasks

urso added v7.12.0 and removed needs_backport PR is waiting to be backported to other branches. labels Dec 24, 2020

urso pushed a commit to urso/beats that referenced this pull request Dec 24, 2020

Add support for customized monitoring API (elastic#22605)

18832e4

(cherry picked from commit 82c11d4)

ycombinator reviewed Dec 29, 2020

View reviewed changes

urso pushed a commit that referenced this pull request Dec 30, 2020

Cherry-pick #22605 to 7.x: Add support for customized monitoring API (#…

c04dd7f

…23279) Co-authored-by: Peter Deng <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for customized monitoring API #22605

Add support for customized monitoring API #22605

newly12 commented Nov 17, 2020 •

edited

Loading

newly12 commented Nov 17, 2020

elasticmachine commented Nov 17, 2020

elasticmachine commented Nov 17, 2020 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

`Terraform Apply on x-pack/metricbeat/module/aws`

`Terraform Apply on x-pack/metricbeat/module/aws`

Test stats 🧪

sayden commented Nov 18, 2020 •

edited

Loading

newly12 commented Nov 18, 2020

vjsamuel commented Nov 25, 2020

exekias commented Nov 26, 2020

newly12 commented Nov 30, 2020

urso Dec 10, 2020

newly12 Dec 11, 2020

newly12 Dec 11, 2020

urso Dec 10, 2020

newly12 Dec 11, 2020

newly12 Dec 16, 2020

urso left a comment

urso Dec 15, 2020

urso Dec 15, 2020

urso Dec 15, 2020

urso Dec 15, 2020

urso Dec 15, 2020

urso commented Dec 17, 2020

urso commented Dec 19, 2020

newly12 commented Dec 22, 2020

ycombinator Dec 29, 2020

newly12 Jan 4, 2021

Add support for customized monitoring API #22605

Add support for customized monitoring API #22605

Conversation

newly12 commented Nov 17, 2020 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Use cases

newly12 commented Nov 17, 2020

elasticmachine commented Nov 17, 2020

elasticmachine commented Nov 17, 2020 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Steps errors

Terraform Apply on x-pack/metricbeat/module/aws

Terraform Apply on x-pack/metricbeat/module/aws

💚 Flaky test report

Test stats 🧪

sayden commented Nov 18, 2020 • edited Loading

newly12 commented Nov 18, 2020

vjsamuel commented Nov 25, 2020

exekias commented Nov 26, 2020

newly12 commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urso commented Dec 17, 2020

urso commented Dec 19, 2020

newly12 commented Dec 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newly12 commented Nov 17, 2020 •

edited

Loading

elasticmachine commented Nov 17, 2020 •

edited by jenkins-beats-ci bot

Loading

`Terraform Apply on x-pack/metricbeat/module/aws`

`Terraform Apply on x-pack/metricbeat/module/aws`

sayden commented Nov 18, 2020 •

edited

Loading