Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new(plugin_api): add plugin metrics support #1828
new(plugin_api): add plugin metrics support #1828
Changes from all commits
5be9530
c85aef5
2ccdd8e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this linux only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libs_metrics_collector
is currently compiled on linux only (see https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/metrics_collector.h#L82)I think the reason behind is that we had linux-only metrics.
However, with this PR that's not the case anymore, so we should definitely enable metrics in other platforms too.
I'm working on that, but maybe it's better to open another PR when I'm done :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of simplicity as macOS and Windows were / are not a primary metrics use case. In more detail it's because of how we compute CPU usage etc, which is Linux specific.
I believe there is no harm to refine what actually only needs to be linux. @mrgian does this help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @incertum Thank you!
I believe we just need to rearrange
#ifdef __linux__
directives.The only hiccup I had is in https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/metrics_collector.cpp#L35-L56
That uses designated initialization (explicit array indexes) which is not supported by MSVC.
However I don't think removing these explicit indexes is a big problem, the comment on top should be enough for contributors to understand that
metrics_unit_name_mappings_prometheus
andmetrics_metric_type_name_mappings_prometheus
need to stay in sync with the enums inmetrics_v2.h
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope, but have we ever thought about using a client library for prometheus, like: https://github.com/jupp0r/prometheus-cpp ? @incertum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FedeDP see the previous discussions; that was the whole point of not adding another lib as we have a more unique use case with all of our different metrics spanning multiple areas of the code base, plus that lib seems not very suitable and supported enough IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Melissa! I probably lost that part :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrgian mind elaborating
However I don't think removing these explicit indexes is a big problem,
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mrgian@cddf7b7
👆 @incertum This is what I've done to make
libs_metrics_collector
compile on Windows.Notice how I had to remove designation initializers from
metrics_unit_name_mappings_prometheus
, it allows to compile with MSVC and the code behavior stays the same.However we now have to make sure
metrics_unit_name_mappings_prometheus
has the same ordering as the values inmetrics_v2_value_unit
inmetrics_v2.h
. This comment should be enough to make contributors aware of this.The same applies for
metrics_metric_type_name_mappings_prometheus
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe ok for now since we don't have much time left for the next release.