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

prometheus: 2.27.1 -> 2.30.3 #141477

Merged
merged 1 commit into from
Nov 7, 2021
Merged

Conversation

eyJhb
Copy link
Member

@eyJhb eyJhb commented Oct 13, 2021

Motivation for this change

Needs update, there are some syntax shown on ie. blackbox-exporter that does not work on the current version of prometheus (I think).

Things done

I have updated the version numbers + hashes.
However, they have changed yarn out with npm, which has caused some headache.
They have also added a module system, that is currently working OK with a hack that needs to be changed...

I have ran the prometheus tests and they pass without any issues.

TODOs

  • Cleanup code, ie. remove unused packages, buildInputs, etc.
  • Update webui update script
  • 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 via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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.

@eyJhb
Copy link
Member Author

eyJhb commented Oct 15, 2021

Copying my text from Matrix to @K900 to here, as it seems quite relevant. :)

if you can validate that it runs and works in a setup that you have used with it, it would be great. I have currently set it up, and it's running fine on my instance. However, there is a dirty hack in the current PR, but I don't really see any other way.

They have implemented a "module" system, that seems very hacky. But seeing as I am building the only module they currently have (codemirror-promql), then I don't think it makes sense, to actually run their build system.

You can see the hack here - https://github.com/NixOS/nixpkgs/pull/141477/files#diff-ca9e5cc95e3ef1d454502e3df0bd0c621434832f1f9feef52dac9a970a5c2100R83

Also, I think that some of the things in the patchPhase, should actually just be in the configure phase. Ie. all the symbolic links, should maybe just be in configure phase. And the one where I override the build modules script, should be in the patch phase.

TL;DR I am currently running this in my own setup, but would love anyone that could try it in their own current setup to validate it. I need some feedback on the hack I have made (check link above), and feedback on how to split up the patchPhase. I have already given a suggestion.

@numkem
Copy link
Contributor

numkem commented Oct 15, 2021

It works great for me.

One thing missing that I know is the option services.prometheus.scrapeConfigs.http_sd_configs it's been added since.

@eyJhb eyJhb changed the title WIP: prometheus: 2.27.1 -> 2.30.3 prometheus: 2.27.1 -> 2.30.3 Oct 18, 2021
@eyJhb
Copy link
Member Author

eyJhb commented Oct 22, 2021

It works great for me.

One thing missing that I know is the option services.prometheus.scrapeConfigs.http_sd_configs it's been added since.

Think that should be added in a separate PR, if you have the time you could make one? :)

@dasJ dasJ requested a review from Ma27 October 22, 2021 10:36
@yrd
Copy link
Member

yrd commented Oct 23, 2021

@eyJhb I started working on the new config changes, see #142654

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Are there new releases of exporters that are broken according to you (e.g. blackbox-exporter)? if yes, these should be updated in here as well.

};

goPackagePath = "github.com/prometheus/prometheus";

webui = mkYarnPackage {
codemirrorNode = import ./webui/codemirror-promql {
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried generating a yarn.lock on your own and use this? IIRC node2nix causes way more work than yarn2nix does in most cases, so I'd prefer to keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we were to go this route, shouldn't we create a yarn.lock from the upstream lockfile? Seems like it wouldn't be a good idea, to run different versions than upstream are

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should stick with yarn, but move to npm.
Looked at this thread here - prometheus/prometheus#9198 which also links this thanos-io/thanos#4562 and it seems to be a general move.

There are some dependencies yarn can't handle as well, that npm handles well. So we could end up having a different set of dependencies than upstream, if we continue to use yarn.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. If upstream is moving to npm we should do so as well.

@eyJhb
Copy link
Member Author

eyJhb commented Oct 23, 2021

Are there new releases of exporters that are broken according to you (e.g. blackbox-exporter)? if yes, these should be updated in here as well.

I am not sure, an unsure where to check it. I run the node-exporter and blackbox-exporter, those both work.

@basvandijk
Copy link
Member

@GrahamcOfBorg test prometheus

Copy link
Member

@basvandijk basvandijk left a comment

Choose a reason for hiding this comment

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

LGTM!

@basvandijk
Copy link
Member

basvandijk commented Nov 4, 2021

@eyJhb I'm trying to upgrade to the latest prometheus-2.31.0 and I noticed they moved the web/ui package to npm workspaces. Any idea how to update your pkgs/servers/monitoring/prometheus/webui/update-web-deps.sh to this new way?

@eyJhb
Copy link
Member Author

eyJhb commented Nov 4, 2021

@eyJhb I'm trying to upgrade to the latest prometheus-2.31.0 and I noticed they moved the web/ui package to npm workspaces. Any idea how to update your pkgs/servers/monitoring/prometheus/webui/update-web-deps.sh to this new way?

It just seems to be sugar on top of npm, and nothing more. Currently it just builds the two components independently, and then fixes the paths.

Should that be any issue? The webui works fine when I tested it :) - Are you available on Matrix for discussing any miscommunication? :)

@basvandijk
Copy link
Member

It just seems to be sugar on top of npm, and nothing more. Currently it just builds the two components independently, and then fixes the paths.

The issue is that they now have a single package-lock.json file in web/ui instead of two separate ones in web/ui/module/codemirror-promql and web/ui/react-app/.

Should that be any issue? The webui works fine when I tested it :)

At the moment the package doesn't build. But maybe I can try just using that single package-lock.json for the two packages.

Are you available on Matrix for discussing any miscommunication? :)

I'm not and TIL about Matrix. I'll look into signing up. Where can I do that?

@basvandijk
Copy link
Member

It seems this is ready to merge. We can do the 2.31.0 upgrade in another PR. Merging in now...

@basvandijk basvandijk merged commit 3e019f7 into NixOS:master Nov 7, 2021
@eyJhb
Copy link
Member Author

eyJhb commented Nov 8, 2021

It seems this is ready to merge. We can do the 2.31.0 upgrade in another PR. Merging in now...

Oh, didn't notice a new version had come out. Sorry!
Hopefully it can be fixed soon. Seems annoying that Prometheus cannot settle on anything, and keeps changing it around.
But what can one do ... :)

Thanks for merging @basvandijk ! :)

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.

6 participants