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

services/prometheus/exporters: add scaphandre #239803

Merged
merged 1 commit into from Jun 28, 2023

Conversation

gaelreyrol
Copy link
Contributor

@gaelreyrol gaelreyrol commented Jun 25, 2023

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.

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

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

);
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
Copy link
Contributor Author

gaelreyrol commented Jun 27, 2023

@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
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
18 checks passed
@gaelreyrol gaelreyrol deleted the prometheus-scaphandre-exporter-init branch June 28, 2023 07:50
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 🙃.

} {
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.

} {
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?

} {
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

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

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

@gaelreyrol
Copy link
Contributor Author

gaelreyrol commented Jun 29, 2023

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants