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

fix(outputs.nebius_cloud_monitoring): replace reserved label names #13597

Merged
merged 9 commits into from
Jul 14, 2023
Merged

fix(outputs.nebius_cloud_monitoring): replace reserved label names #13597

merged 9 commits into from
Jul 14, 2023

Conversation

abrekhov
Copy link
Contributor

@abrekhov abrekhov commented Jul 11, 2023 •
edited

Required for all PRs

resolves #13596

powersj
powersj reviewed Jul 11, 2023
View reviewed changes
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Hi!

Thanks for the quick fix. Can you also please add a section to the plugin README called "Reserved Labels" and explain that tags with the key "name" are renamed.

One question inline as well about what this ultimately looks like in Nebius.

plugins/outputs/nebius_cloud_monitoring/nebius_cloud_monitoring.go Outdated Show resolved Hide resolved
@powersj powersj self-assigned this Jul 11, 2023
@powersj powersj added the waiting for response waiting for response from contributor label Jul 11, 2023
@abrekhov @Hipska
fix: Apply suggestions from code review
af580e9 
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
Hipska
Hipska reviewed Jul 12, 2023
View reviewed changes
plugins/outputs/nebius_cloud_monitoring/nebius_cloud_monitoring_test.go Outdated
require.NoError(t, err)
require.Len(t, message.Metrics, 1)
require.Equal(t, "cluster_value", message.Metrics[0].Name)
require.Equal(t, "accounts-daemon.service", message.Metrics[0].Labels["_name"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the check better be if _name label exists?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what is done, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, as it now will panic or error out if message.Metrics[0].Labels doesn't have an item called _name.

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 right. We need panic if input has name label, but there is no _name in tt.got. Am I correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

No we don't want that, we want the test to fail, not to crash.

So the test should include something like:

require.Contains(t, message.Metrics[0].Labels, "_name")

@abrekhov
Copy link
Contributor Author

@powersj @Hipska
image
That is how it looks like.

But I don't understand why last version hasn't pass intagration test.

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Jul 12, 2023
@abrekhov
Copy link
Contributor Author

image

It seems to me that _name is better also.

@powersj powersj assigned srebhan and unassigned powersj Jul 12, 2023
@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 Jul 12, 2023
srebhan
srebhan reviewed Jul 12, 2023
View reviewed changes
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.

@abrekhov thanks for the fix! I do have some minor comments...

plugins/outputs/nebius_cloud_monitoring/nebius_cloud_monitoring_test.go Outdated Show resolved Hide resolved
plugins/outputs/nebius_cloud_monitoring/nebius_cloud_monitoring_test.go Outdated Show resolved Hide resolved
plugins/outputs/nebius_cloud_monitoring/README.md Outdated Show resolved Hide resolved
@telegraf-tiger
Copy link
Contributor

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 doesn't significantly change the Telegraf binary size (less than 1%)

📦 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

srebhan
srebhan approved these changes Jul 14, 2023
View reviewed changes
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 for the fix @abrekhov!

@srebhan srebhan added fix pr to fix corresponding bug cloud Issues or requests around cloud environments plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Jul 14, 2023
@srebhan srebhan merged commit 3670971 into influxdata:master Jul 14, 2023
23 checks passed
@github-actions github-actions bot added this to the v1.27.3 milestone Jul 14, 2023
@abrekhov
Copy link
Contributor Author

@srebhan @powersj @Hipska guys thanks for quick review!

@abrekhov abrekhov deleted the nebius branch July 17, 2023 08:05
powersj pushed a commit that referenced this pull request Jul 31, 2023
@abrekhov @powersj
fix(outputs.nebius_cloud_monitoring): Replace reserved label names (#…
22a867b 
…13597)

Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
(cherry picked from commit 3670971)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hipska Hipska Hipska requested changes

@powersj powersj powersj approved these changes

@srebhan srebhan srebhan approved these changes

Assignees

@srebhan srebhan

Labels
cloud Issues or requests around cloud environments fix pr to fix corresponding bug plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins 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
Milestone
v1.27.3
Development

Successfully merging this pull request may close these issues.

Nebius Cloud Monitoring has reserved "name" tag, so it must be replaced
4 participants
@abrekhov @Hipska @powersj @srebhan