Skip to content
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

Remove all mentions of Sysdig and Falco in our codebase #1346

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

Molter73
Copy link
Collaborator

@Molter73 Molter73 commented Sep 25, 2023

Description

This is a long due change, all mentions of Sysdig and Falco have been replaced by a more generic system_inspector name. No functional change has been made, the PR is purely organizational.

Checklist

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Testing Performed

CI should be enough, since things are just being shuffled around.

@openshift-ci
Copy link

openshift-ci bot commented Sep 25, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@github-actions
Copy link

Kernel Method Without Collector Time (secs) With Collector Time (secs) Baseline median (secs) Collector median (secs) PValue

@Molter73 Molter73 force-pushed the mauro/remove-sysdig-references branch 2 times, most recently from 6cb4a33 to 5264462 Compare November 9, 2023 13:47
@Molter73 Molter73 force-pushed the mauro/remove-sysdig-references branch 3 times, most recently from 79565ad to 9fb5f73 Compare February 21, 2024 12:21
This is a long due change, all mentions of Sysdig and Falco have been
replaced by a more generic system_inspector name.
@Molter73 Molter73 force-pushed the mauro/remove-sysdig-references branch from 9fb5f73 to e1f53ba Compare February 21, 2024 16:48
@Molter73 Molter73 marked this pull request as ready for review February 27, 2024 16:38
@Molter73 Molter73 requested a review from a team as a code owner February 27, 2024 16:38
Copy link
Contributor

@erthalion erthalion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good and makes sense. Few comment below.

@@ -1,10 +1,9 @@
#include "SysdigService.h"
#include "gmock/gmock.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, it wasn't used here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't, the tests use full fledged Falco objects instead of mocks, they just doesn't instruct the inspector to start collecting data.

@@ -315,10 +315,10 @@ Note that the `[syscall]` suffix in a metric name means that it is instanciated

Note that if ProcfsScraper is unable to open /proc it is not able to open any of the subdirectories, but only procfs_could_not_open_proc_dir will be incremented in that case.

### Falco timers per syscall
### system_inspector timers per syscall
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth mentioning somewhere before this point, that system_inspector in the current incarnation is actually nothing more than Falco inspector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I'll find a place for it.


namespace collector {

class SysdigService;
class Service;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nitpick, but can this be referred to by the fully qualified name, with the namespace? Otherwise the name "Service" is quite generic, and if it could be consistently used everywhere with the "system_inspector" (as everywhere else in this PR), that would be great.

@@ -0,0 +1,62 @@
set(COLLECTOR_LIB_SRC_FILES
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about switching from *.cpp to the exact list of files? Might be coming as a surprise for unprepared developers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just prefer the explicit way of listing source files with cmake, (I believe this is also the recommended way to do things, since globbing *.cpp can cause things like new files not being added to already configured builds), but you are probably right that this is not the place to make this change, I'll roll it back and we can circle back to it at a later point.

@Molter73 Molter73 merged commit 433c054 into master Mar 22, 2024
49 checks passed
@Molter73 Molter73 deleted the mauro/remove-sysdig-references branch March 22, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants