[Metricbeat] Migrate Aerospike namespace Metricset to use ReporterV2 interface #10984
Conversation
sayden
added
Metricbeat
|
Error in ES module seems unrelated https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-linux/beat=metricbeat,label=linux-immutable/5462/console |
| @@ -98,11 +100,11 @@ func (m *MetricSet) Fetch() ([]common.MapStr, error) { | |||
| "name": node.GetName(), | |||
| } | |||
|
|
|||
| events = append(events, data) | |||
| reporter.Event(mb.Event{MetricSetFields: data}) | |||
There was a problem hiding this comment.
We could interrupt this loop to avoid further queries if reporter.Event() is returning false.
There was a problem hiding this comment.
Is that a good idea? I mean, let's face some examples:
- Sending 100 events and 1 of them failed because of a network issue (for the purpose of the example). 99 events would arrive.
- 1 event failed so I don't send the next 99.
- 100 event fails and we have 100 error messages. While they are not harmful, it is a bit annoying to receive 100 times the same error message. 100 events didn't arrive, of course.
- A mix, where we only send some of the events and some not mixed with some error messages.
So, my assumption after you comment is that we prefer to get those 99 events and produce an error message in each case.
WDYT? 🙂
There was a problem hiding this comment.
The thing is that it doesn't return false because of a network issue, it returns false in a totally deterministic way if the metricset has been stopped, and in that case all the following calls will also fail. Following with your example, if you have 100 namespaces, and the second one fails to be reported, the other 97 will also fail, so it does 97 requests for namespace metrics that are not going to be reported.
Remember this thread.
In any case I think this is not such a big deal, so as you prefer, but I find the possibility of handling this as an advantage of using reporter V2.
There was a problem hiding this comment.
Yeah, I completely forgot about that thread. I just wanted to be sure that we were taking into account all possible scenarios. I'll modify it. Thanks Jaime 🙂
|
jenkins, test this |
|
@sayden Seeing this PR is approved do you plan to merge it? |
sayden
force-pushed
the
migration/mb/reporterv2/aerospike/namespace
branch
from
638df4d
to
4061393
Compare
|
I took advantage to add the error interface too. I couldn't use the new testing framework here. |
Refer to #10774 for more info