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

chore(deps): Bump prometheus from v1.8.2 to v2.42.0 #13225

Merged
merged 3 commits into from May 3, 2023

Conversation

LukeWinikates
Copy link
Contributor

This PR updates the prometheus package to v0.42, currently the second-newest version.

Bumping prometheus allows us to also bump a few kubernetes-related packages, and this PR also includes code changes required for compatibility with those versions.

This PR is adapted from: #13159
That PR tries to update to v0.43, but v0.43 causes build errors that can't currently be resolved.

@LukeWinikates LukeWinikates changed the title Bump prometheus v042 chore: bump prometheus version to v0.42 May 2, 2023
- update types in kube_inventory input plugin tests
- update kubernetes input plugin to add error handler for AddEventHandler call
p.watchPod(ctx, client)
err = p.watchPod(ctx, client)
if err != nil {
p.Log.Errorf("Unable to monitor pod: %s", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this in the other PR and it made me pause. We are ignoring these errors right now, is this going to cause instances where things are working today to stop working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's fair. I wouldn't have touched this except for the linter requesting some kind of error handling, now that the underlying API returns an error.

I'll update the PR to log the error and continue, so that the behavior should be the same as what currently happens, with the addition of a warning message in the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok:

  • switched to p.Log.Warnf, thinking that this is a non-fatal warning.
  • no longer calling <-ctx.Done() early if AddEventHandler returns an error. Keeping the control flow the same as it was, with the addition of capturing the error and returning it from watchPod

plugins/inputs/prometheus/kubernetes.go Show resolved Hide resolved
LukeWinikates and others added 2 commits May 2, 2023 13:15
Co-authored-by: Joshua Powers <powersj@fastmail.com>
- update types in kube_inventory input plugin tests
- update kubernetes input plugin to add error handler for AddEventHandler call
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented May 2, 2023

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

⚠️ This pull request increases the Telegraf binary size by 1.32 % for linux amd64 (new size: 175.1 MB, nightly size 172.8 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label May 2, 2023
@srebhan srebhan changed the title chore: bump prometheus version to v0.42 chore(deps): Bump prometheus from v1.8.2 to v2.42.0 May 3, 2023
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @LukeWinikates for the PR and for fixing the remaining issues!

@srebhan srebhan merged commit e9f55a8 into influxdata:master May 3, 2023
25 checks passed
powersj pushed a commit that referenced this pull request May 22, 2023
Dependency version 0.42.0 corresponds to release version 2.42.0 see [prometheus versioning description](https://github.com/prometheus/prometheus#prometheus-code-base) for details.

(cherry picked from commit e9f55a8)
@msenskp
Copy link

msenskp commented May 27, 2023

@LukeWinikates A naive question as I do not know go lang: I see the Prometheus releases are tagged both with v0.42.0 and v2.42.0 at the same time. I am not sure why it is versioned like this but is it possible to upgrade the versions into v2.4+ as the commit message itself specify here https://github.com/LukeWinikates/telegraf/blob/7d3b725f62c27bbbf6566d081ba08f0b5d73d56f/go.mod#L147

I noticed that this is confusing some CVE image scans for some tools such as this
image
Otherwise it sounds like we are moving backward from 1.8.2... to 0.42.0

@LukeWinikates
Copy link
Contributor Author

@msenskp there's more context on the prometheus versions in #13159 and at https://github.com/prometheus/prometheus#prometheus-code-base

As I understand it, prometheus/prometheus isn't really meant to be used as a library (it's an application codebase), and so the code does not follow SemVer rules about API stability in the way that golang tooling expects. To get around the problem, the Prometheus project 'reset' the version numbers for the go package to all begin with 0, even though the corresponding application is 2.x.x. This solved the SemVer problem because 0. versions in SemVer are considered pre-release versions and are allowed to have breaking API changes in minor versions. But it does mean that you have to "downgrade" to "upgrade". Hopefully there is a way for you to mute that warning in the CVE scanner.

The longer term solution is probably this: #13165 .

@msenskp
Copy link

msenskp commented May 27, 2023

@LukeWinikates thank you for the insight. Also glad to hear that there is a long term solution plan with a smaller footprint on the dependencies.

@srebhan srebhan added this to the v1.26.3 milestone Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants