search

falcosecurity / falco

Cloud Native Runtime Security
https://falco.org
Apache License 2.0
7.29k stars 897 forks source link

new(metrics): enable plugins metrics #3228

Closed mrgian closed 3 months ago

mrgian commented 4 months ago

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(metrics): enable plugins metrics
poiana commented 4 months ago

LGTM label has been added.

Git tree hash: 7727207ee99834204b7925d641c637b454b07501

incertum commented 4 months ago

/milestone 0.39.0

FedeDP commented 4 months ago

@incertum don't we need to implement this for falco_metrics too? (ie: prometheus)? /hold

@mrgian

FedeDP commented 4 months ago

/milestone 0.38.1

incertum commented 3 months ago

@incertum don't we need to implement this for falco_metrics too? (ie: prometheus)? /hold

@mrgian

Good call @FedeDP -- overlooked that, yes @mrgian could you add the changes to the webserver as well?

mrgian commented 3 months ago

@incertum don't we need to implement this for falco_metrics too? (ie: prometheus)? /hold

@mrgian

Good call @FedeDP -- overlooked that, yes @mrgian could you add the changes to the webserver as well?

It shouldn't be needed since libs_metrics_collector is already initialized with the METRICS_V2_PLUGINS flag. https://github.com/falcosecurity/falco/blob/master/userspace/falco/falco_metrics.cpp#L64

~But seems like plugin-provided metrics cause a segfault when a request is made to the webserver. The issue may lie in prometheus_metrics_converter::convert_metric_to_text_prometheus in the libs, however I'm still searching for the root cause of this.~

@FedeDP @incertum

mrgian commented 3 months ago

False alarm on the segfault! I was using an old plugin as test 🤦

As for enabling plugin metrics in the webserver, we don't need to do that since since libs_metrics_collector is already initialized with the METRICS_V2_PLUGINS flag.

We can think of changing the namespace for plugin-provided metrics to something like plugin or falco_plugin in: https://github.com/falcosecurity/falco/blob/master/userspace/falco/falco_metrics.cpp#L227

WDYT?

incertum commented 3 months ago

False alarm on the segfault! I was using an old plugin as test 🤦

As for enabling plugin metrics in the webserver, we don't need to do that since since libs_metrics_collector is already initialized with the METRICS_V2_PLUGINS flag.

We can think of changing the namespace for plugin-provided metrics to something like plugin or falco_plugin in: https://github.com/falcosecurity/falco/blob/master/userspace/falco/falco_metrics.cpp#L227

WDYT?

This is a very good idea, big +1:

leogr commented 3 months ago

Left a small question: everything else sgtm! Thanks both Melissa and Gian for this!

Otherwise, SGTM too!

poiana commented 3 months ago

LGTM label has been added.

Git tree hash: 93fb28e758f3a0465fa6340cbfc76924ad9a2e85

incertum commented 3 months ago

@FedeDP CI is still not cooperating.

FedeDP commented 3 months ago

Oh of course, we need to bump libs to master too! I will open the bump PR so that @mrgian can then rebase this one and we are good to go!

FedeDP commented 3 months ago

@mrgian can you rebase on master to fix the CI?

mrgian commented 3 months ago

@FedeDP Rebased :)

poiana commented 3 months ago

LGTM label has been added.

Git tree hash: 685f492ca5843d053268c84ba05ed50235318695

poiana commented 3 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, mrgian

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/falcosecurity/falco/blob/master/OWNERS)~~ [FedeDP,incertum] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
incertum commented 3 months ago

Btw @mrgian we need to update the website now ... https://falco.org/docs/metrics/falco-metrics/#plugins-metrics CC @LucaGuerra it's gonna be interesting to explain that the plugins metrics are custom (no default ones) vs we don't truly support the other metrics for plugins atm 🙃 because of some regressions ...

mrgian commented 3 months ago

@incertum I'm currently working on the website to document the latest plugin API features. I'll update the page about Falco metrics too :)

FedeDP commented 3 months ago

/unhold