Add envoyproxy module #7569
Add envoyproxy module #7569
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? |
| return events, nil | ||
| } | ||
|
|
||
| func findStats(data common.MapStr, response []byte, re_name string) common.MapStr { |
There was a problem hiding this comment.
don't use underscores in Go names; func parameter re_name should be reName
| }, nil | ||
| } | ||
|
|
||
| func (m *MetricSet) Fetch() (common.MapStr, error) { |
There was a problem hiding this comment.
exported method MetricSet.Fetch should have comment or be unexported
| http *helper.HTTP | ||
| } | ||
|
|
||
| func New(base mb.BaseMetricSet) (mb.MetricSet, error) { |
There was a problem hiding this comment.
exported function New should have comment or be unexported
| ) | ||
| } | ||
|
|
||
| type MetricSet struct { |
There was a problem hiding this comment.
exported type MetricSet should have comment or be unexported
There was a problem hiding this comment.
Thank you for this awesome contribution @berfinsari! 🎉 🎉
I left some minor comments, but this is looking great overall.
System test is failing on some undocumented fields, can you please have a look?
Exception: Key 'envoyproxy.server.http.async-client.no_route:' found in event is not documented!
| } | ||
| for i := 0; i < len(stats); i++ { | ||
| event := findStats(data, response, stats[i]) | ||
| events, err = schema.Apply(event) |
There was a problem hiding this comment.
Isn't this overriding the events var on every pass? It sounds like you could call it just once after the loop (using the loop to populate data only)
There was a problem hiding this comment.
Also, have a look to Apply options, the default behavior is: all fields are required, you will get an error if any of them is missing. I'm wondering if some fields may change between releases. A more relaxed check would be: schema.Apply(event, schema.FailOnRequired). Pinging @jsoriano on this
There was a problem hiding this comment.
There is an example of required fields here.
There was a problem hiding this comment.
Thanks for your review. I think now overriding issue is solved but I am not sure which fields are required. I am handle with this.
| } | ||
|
|
||
| func findStats(data common.MapStr, response []byte, reName string) common.MapStr { | ||
| re := regexp.MustCompile(reName) |
There was a problem hiding this comment.
you can avoid recompiling this on every fetch by moving from a local stats := []string to a static var var stats []*regexp.Regexp
| "watchdog_mega_miss": c.Int("watchdog_mega_miss"), | ||
| "watchdog_miss": c.Int("watchdog_miss"), | ||
| }, | ||
| "listener": s.Object{ |
There was a problem hiding this comment.
Is the list of listeners and http objects dynamic? If it is maybe Schema (as is now) is not the best option to parse these fields.
There was a problem hiding this comment.
What do you recommend to parse these fields? Do you have an example for this?
There was a problem hiding this comment.
I see some options here:
- Don't use
schemato parse these fields, you can take a look to other modules that don't make use of this package, as haproxy. - Complete this PR without these fields, and we can think in the meantime how to support these dynamic objects with
schema. Onceschemais ready, support for these metrics can be added in a new PR.
Another intermediary solution would be to use schema only for the non-dynamic fields, and use other mechanisms to fill the rest of fields.
I'm fine with any of these options.
There was a problem hiding this comment.
Thank you for the reply! I removed dynamic fields and updated docs for the non-dynamic fields. In the meantime, I will look to other modules for dynamic fields.
There was a problem hiding this comment.
Nice, I'm thinking that probably listeners and http events should be collected from a different metricset, and send different events. Let's finish this PR and then we can continue thinking on these metrics on dynamic elements 🙂
| } | ||
| for i := 0; i < len(stats); i++ { | ||
| event := findStats(data, response, stats[i]) | ||
| events, err = schema.Apply(event) |
There was a problem hiding this comment.
There is an example of required fields here.
…finsari/beats into metricbeat-envoyproxy-module
|
@exekias Did you have any chance to check latest changes? |
|
jenkins, test this |
|
Thank you so much for this contribution @berfinsari! Do you happen to have dashboards for this module? It would be awesome to include them in the distribution, so all users benefit from them! |
The envoyproxy module collects metrics.