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.

Sign up for GitHub

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

Jump to bottom

services/prometheus/exporters: add scaphandre #239803

Merged
merged 1 commit into from
Jun 28, 2023
Merged

services/prometheus/exporters: add scaphandre #239803

merged 1 commit into from
Jun 28, 2023

Conversation

gaelreyrol
Copy link
Contributor

@gaelreyrol gaelreyrol commented Jun 25, 2023 •
edited
Loading

Description of changes

This PR add scaphandre package to prometheus exporters.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 25, 2023
@gaelreyrol gaelreyrol force-pushed the prometheus-scaphandre-exporter-init branch from c0192ee to f5ea447 Compare June 25, 2023 18:17
@github-actions github-actions bot added 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Jun 25, 2023
@gaelreyrol gaelreyrol force-pushed the prometheus-scaphandre-exporter-init branch 2 times, most recently from 3f94e09 to 9036413 Compare June 25, 2023 18:22
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 25, 2023
@gaelreyrol gaelreyrol force-pushed the prometheus-scaphandre-exporter-init branch from 9036413 to 4876a3b Compare June 26, 2023 17:43
@gaelreyrol gaelreyrol mentioned this pull request Jun 26, 2023
Closed
@gaelreyrol gaelreyrol marked this pull request as ready for review June 26, 2023 17:46
@gaelreyrol gaelreyrol requested a review from WilliButz as a code owner June 26, 2023 17:46
@gaelreyrol gaelreyrol self-assigned this Jun 26, 2023
@gaelreyrol gaelreyrol removed 8.has: package (new) This PR adds a new package 10.rebuild-linux: 1-10 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 11.by: package-maintainer This PR was created by the maintainer of the package it changes labels Jun 27, 2023
gaelreyrol
gaelreyrol commented Jun 27, 2023
View reviewed changes
nixos/tests/prometheus-exporters.nix
enable = true;
};
metricProvider = {
boot.kernelModules = [ "intel_rapl_common" ];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently this option will fail on the CI but it does not fail on my host even if ran in QEMU. I added an assertion to make sure it is added explicitly by the user but it seems scaphandre does not produce any error if it is not "really" probed.

Copy link
Contributor Author

@gaelreyrol gaelreyrol Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Well it produces an error but it is not what's tested, should I try to handle it?
image

gaelreyrol
gaelreyrol commented Jun 27, 2023
View reviewed changes
nixos/modules/services/monitoring/prometheus/exporters.nix
);
message = ''
Please enable 'intel_rapl_common' in boot.kernelModules
'';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I should also assert that machine runs on Intel or AMD according to the compatibility page but I don't know how to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to restrict as much as possible the possibility to use Scaphandre without having the proper configuration.

@gaelreyrol gaelreyrol force-pushed the prometheus-scaphandre-exporter-init branch from 4876a3b to 877f76c Compare June 27, 2023 15:54
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 27, 2023
@gaelreyrol gaelreyrol force-pushed the prometheus-scaphandre-exporter-init branch from 877f76c to d64f297 Compare June 27, 2023 17:21
@gaelreyrol
Copy link
Contributor Author

gaelreyrol commented Jun 27, 2023 •
edited
Loading

@GrahamcOfBorg test prometheus-exporters.scaphandre

@gaelreyrol gaelreyrol force-pushed the prometheus-scaphandre-exporter-init branch from d64f297 to cf16bbc Compare June 27, 2023 18:15
@gaelreyrol
Copy link
Contributor Author

@GrahamcOfBorg build scaphandre

@gaelreyrol gaelreyrol force-pushed the prometheus-scaphandre-exporter-init branch from cf16bbc to f63009a Compare June 27, 2023 18:20
@gaelreyrol gaelreyrol requested a review from drupol June 27, 2023 18:22
@gaelreyrol gaelreyrol force-pushed the prometheus-scaphandre-exporter-init branch from f63009a to e11f06a Compare June 27, 2023 18:51
drupol
drupol approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM

@drupol drupol merged commit e1b3f7b into NixOS:master Jun 28, 2023
@gaelreyrol gaelreyrol deleted the prometheus-scaphandre-exporter-init branch June 28, 2023 07:50
ambroisie
ambroisie reviewed Jun 29, 2023
View reviewed changes
Copy link
Contributor

@ambroisie ambroisie left a comment

Choose a reason for hiding this comment

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

I got linked to the commit from Matrix, I think there are some issues with this MR as it was merged 🙃.

nixos/modules/services/monitoring/prometheus/exporters.nix
} {
assertion = cfg.scaphandre.enable -> (pkgs.stdenv.hostPlatform.isx86_64 == true);
message = ''
Only x86_64 host platform architecture is not supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant

Suggested change
Only x86_64 host platform architecture is not supported.
Only x86_64 host platform architecture is supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

And are you sure the (build) host should be x86_64? Or do you mean the (build) target, i.e: the machine that will run the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I mean the build target indeed.

nixos/modules/services/monitoring/prometheus/exporters.nix
} {
assertion = cfg.scaphandre.enable -> ((lib.kernel.whenHelpers pkgs.linux.version).whenOlder "5.11" true).condition == false;
message = ''
A kernel version newer than '5.11' is required. ${pkgs.linux.version}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should explain that the assertion comes from cfg.scaphandre.enable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ambroisie Is there any convention to explain that if comes from a specific config?

nixos/modules/services/monitoring/prometheus/exporters.nix
} {
assertion = cfg.scaphandre.enable -> (builtins.elem "intel_rapl_common" config.boot.kernelModules);
message = ''
Please enable 'intel_rapl_common' in 'boot.kernelModules'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@drupol
Copy link
Contributor

drupol commented Jun 29, 2023

I got linked to the commit from Matrix, I think there are some issues with this MR as it was merged upside_down_face.

Still learning the job of a maintainer every single day! Sorry for the trouble !

@gaelreyrol
Copy link
Contributor Author

@ambroisie What are the issues other than your review comments?

@ambroisie
Copy link
Contributor

ambroisie commented Jun 29, 2023 •
edited
Loading

@gaelreyrol I meant the ones I commented on :-).

@drupol no worries, I don't even have the commit bit myself 😄.

@gaelreyrol gaelreyrol mentioned this pull request Jun 29, 2023
Merged
12 tasks
@gaelreyrol
Copy link
Contributor Author

gaelreyrol commented Jun 29, 2023 •
edited
Loading

@ambroisie I submitted a new PR to apply your comments: #240571

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambroisie ambroisie ambroisie left review comments

@drupol drupol drupol approved these changes

@WilliButz WilliButz Awaiting requested review from WilliButz

Assignees

@gaelreyrol gaelreyrol

Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gaelreyrol @drupol @ambroisie