Aggregation based custom metrics

[alstanchev]

Adds the posibility to define search based custom metrics.

Things can be tagged with predefined values in the config as well as with resolved ones from the things themselves.
All devices that match a filter and have the same tags ar aggregated under one metric instrument.
A cleanup is implemented so that no metric instruments are left after they are no longer used based on last time used

Resolves #1972

[thjaeckle]

@alstanchev I will try to look at the PR soon, thanks :)

In the meantime: the GitHub unit tests build fail because of these command registry tests.
There you probably added a new command + a new response + a new exception - and those must be added to the unit test.

[thjaeckle]

Hi @alstanchev

I did a first round of review.

I found some potential concurrency issues in the added OperatorSearchMetricsProviderActor.
Also, the added dependency to ditto-edge-service is not in line with keeping services only to the minimum dependencies they really need to know about.

Please have a look at my remarks, the concurrency thing is concerning me ..

[thjaeckle]

The overall approach works of course, it however has some significant problems when the provided filter e.g. targets a huge dataset (e.g. millions of things - for which Ditto is built).
Then, the approach will do a lot of "pagination" and put a lot of stress to Ditto, because all of the targeted things will have to be loaded into memory.

It is of course better than doing this "outside" of Ditto, using the HTTP API and doing the aggregation outside .. but it still is not ideal.
I would at least provide a disclaimer to the documentation to use this feature with care.

I would however suggested another way to do those "aggregations", which would be a lot more efficient and resource friendly:
We could use MongoDB aggregations in order to do those, as also suggested in #1525

So on the MongoDB search collection do an aggregation query (sticking to your provided example) of:

db.search.aggregate([
  {
    $group : {
      _id: "$t.attributes.Info.location",
      "sumOnline": {
        $sum: {
          $cond: [ { $gt: ["$t.features.ConnectionStatus.properties.status.readyUntil", "2024-07-03T08:10:00.000Z" ] }, 1, 0]
        }
      },
      "sumOffline": {
        $sum: {
          $cond: [ { $lt: ["$t.features.ConnectionStatus.properties.status.readyUntil", "2024-07-03T08:10:00.000Z" ] }, 1, 0]
        }
      }
    }
  }
])

Which would provide a DB result (without any need to page internally) like:

[
  {
    "_id": "Berlin",
    "sumOnline": 6,
    "sumOffline": 23
  },
  {
    "_id": "Immenstaad",
    "sumOnline": 8,
    "sumOffline": 42
  }
]

So FMPOV this could even simplify the PR a lot .. e.g. the "operator" of Ditto could "just" configure the MongoDB aggregation query to perform in the configuration file.
Maybe with some "sugar" like using Ditto RQL to be able to e.g. use time:now placeholder.

What do you think, @alstanchev ?

[alstanchev]

Awsome review, @thjaeckle, thanks. Sorry for the late reply i was out of office. I realy like the idea to move the aggregation to mongodb. Will look in to your suggestions and come back to discuss it.

[alstanchev]

@thjaeckle i have refactored the whole PR and the aggregation is now done in the db.
Please have a look when you can.

[thjaeckle]

@alstanchev awesome, will do a review..

[thjaeckle]

@alstanchev I did a first deeper review

This looks like a really great approach - I like how you reused the Visitor for just creating an "aggregation visitor" 👍

I added remarks/requests inline.

In general, I think, that it would make sense to update configuration, code and documentation to not name this "custom search metrics" - but e.g. "custom aggregated metrics".
As the Ditto search is not used any longer for obtaining the metrics.

What do you think?

[thjaeckle]

Thanks a lot for the adjustments @alstanchev
I will try to do a last review round the next days :)

[thjaeckle]

Hi @alstanchev

Looks almost good :)
I did a commit, doing some more missed renamings .. but I do not have the permission to push to your branch.
I will attach a patch.

And 2 more things:

• I noticed, when testing this feature, that also a "filter" tag is added to the metric, e.g. I had configured for testing a filter above_8:

my_test_metric{huge="true",bool="false",location="bath",filter="above_8"} 2.0

• I understood that this should not be the case - at least I couldn't read it in the documentation .. is a matching "filter" added on purpose as tag to the metric?
• I also noticed a lot of WARN logs:

2024-09-13 14:09:43,100 WARN  [][] o.e.d.i.u.m.i.t.StartedKamonTimer  - Tried to stop the already stopped timer <aggregate_things_metrics> with segment <overall>.

• Do you also get those WARN logs and could you look into them? I would not want to flood the Ditto search logs with WARN logs like those ..

Apart from that, this is a really awesome feature. Cannot wait to try it out on real data :)

[thjaeckle]

1973-review.patch

[alstanchev]

Great thanks, will look into the WARN logs. There was a double call on stop but i tought i fixed it.

[alstanchev]

@thjaeckle i have applied the changes you have requested and also minor other fix.

[thjaeckle]

@alstanchev did you fix the WARN logs as well?

[alstanchev]

should be, i am not getting it locally. The problem was that stop was called on each element of the stream. Now it is in a .watchTermination() of the stream.

[thjaeckle]

Cool, had another look - all fine for me now :)

Thanks again and a lot for this great addition 👍

Merge whenever you are ready to :)