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

Fix incorrect name and doc for kv_entries metric #13496

Merged
merged 5 commits into from
Aug 29, 2022
Merged

Fix incorrect name and doc for kv_entries metric #13496

merged 5 commits into from
Aug 29, 2022

Conversation

maxb
Copy link

@maxb maxb commented Jun 19, 2022

The name of the metric as registered with the metrics library to provide
the help string, was incorrect compared with the actual code that sets
the metric value - bring them into sync.

Also, the help message was incorrect. Rather than copy the help message
from telemetry.mdx, which was correct, but felt a bit unnatural in the
way it was worded, update both of them to a new wording.

Link to actual code that sets the metric value, for comparison:

[]string{"consul", "state", "kv_entries"},

PR that originally introduced the metric, and the discrepancies fixed here: #11090

The name of the metric as registered with the metrics library to provide
the help string, was incorrect compared with the actual code that sets
the metric value - bring them into sync.

Also, the help message was incorrect. Rather than copy the help message
from telemetry.mdx, which was correct, but felt a bit unnatural in the
way it was worded, update both of them to a new wording.
@maxb maxb requested a review from a team as a code owner June 19, 2022 11:10
@github-actions github-actions bot added the type/docs Documentation needs to be created/updated/clarified label Jun 19, 2022
Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

Docs part of this PR is 👍

@Amier3
Copy link
Contributor

Amier3 commented Jun 21, 2022

Thanks adam! Looks straightfoward to me as well, i'll bring this up and hopefully we can just approve the merge tomorrow when the team meets to discuss the prometheus issues

@maxb
Copy link
Author

maxb commented Jun 26, 2022

I think this is going to experience a merge conflict with my other open PR touching telemetry.mdx - #13516.

Could you get that other PR - which has been approved already - merged in, so that I can resolve the conflict?

@Amier3
Copy link
Contributor

Amier3 commented Jun 27, 2022

@maxb

Just merged the last one so you're free to add changes.

Also an update on your PRs: I had to push back discussions on all your changes since last week was our annual european conference. Will hopefully get you back feedback shortly.

@maxb
Copy link
Author

maxb commented Jun 27, 2022

Sure, I appreciate the complicated stuff is going to take a while to work through :-)

Meanwhile, this little 3-liner now has conflicts resolved.

@maxb
Copy link
Author

maxb commented Jun 27, 2022

Please could I get a pr/no-metrics-test label for this PR?

Please could I get a pr/no-changelog label for this PR, unless you think even something this minor warrants a changelog entry?

@Amier3 Amier3 added the pr/no-changelog PR does not need a corresponding .changelog entry label Jun 29, 2022
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@maxb
Copy link
Author

maxb commented Aug 14, 2022

@freddygv @Amier3 Do you think this could get merged in now?

@maxb
Copy link
Author

maxb commented Aug 14, 2022

Hmm, actually, it looks like there is an error in a previous merge, let me fix that...

@maxb
Copy link
Author

maxb commented Aug 14, 2022

Corrected. Please merge if you are happy with this.

@freddygv
Copy link
Contributor

Hi @maxb we're having an issue with some integration tests not running on forked repos, and I can't override that.

We're looking into how to address this so that the integration tests run and we can merge your PR.

@freddygv
Copy link
Contributor

@maxb Could you pull in the latest changes from the main branch? That should allow for the integration tests to run

@maxb
Copy link
Author

maxb commented Aug 29, 2022

@maxb Could you pull in the latest changes from the main branch? That should allow for the integration tests to run

Done - all CircleCI checks green now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-changelog PR does not need a corresponding .changelog entry pr/no-metrics-test type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants