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

metrics: introduce client config to include meta as part of the base label #23964

Merged

Conversation

mvegter
Copy link
Contributor

@mvegter mvegter commented Sep 15, 2024

Took a stab at the following tickets / discussions :

First time opening a pull request using Go , feedback is much appreciated !

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @mvegter! This is looking pretty good. I've left some comments. Additionally, can you:

  • run make cl to add a changelog entry
  • add documentation for this new config value to website/content/docs/configuration/client.mdx. This should warn about the potential impact to cardinality.

Thanks for this PR!

client/allocrunner/taskrunner/task_runner_test.go Outdated Show resolved Hide resolved
client/allocrunner/taskrunner/task_runner_test.go Outdated Show resolved Hide resolved
client/allocrunner/taskrunner/task_runner_test.go Outdated Show resolved Hide resolved
client/allocrunner/taskrunner/task_runner_test.go Outdated Show resolved Hide resolved
client/allocrunner/taskrunner/task_runner_test.go Outdated Show resolved Hide resolved
client/config/config.go Outdated Show resolved Hide resolved
client/allocrunner/taskrunner/task_runner.go Outdated Show resolved Hide resolved
@mvegter
Copy link
Contributor Author

mvegter commented Sep 30, 2024

Thanks for the feedback @tgross , I have updated the telemetry.mdx instead of the client.mdx as there was no mention of metrics nor telemetry on that page.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Looking great @mvegter! Just a few more small things and this should be good-to-go.

command/agent/config.go Outdated Show resolved Hide resolved
website/content/docs/configuration/telemetry.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@aimeeu aimeeu left a comment

Choose a reason for hiding this comment

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

Thanks @mvegter for the docs update as part of your code PR! Thanks also Tim for reviewing. I made a minor suggestion so the docs content conforms to our style guide rule to use present tense.

website/content/docs/configuration/telemetry.mdx Outdated Show resolved Hide resolved
@mvegter mvegter force-pushed the mvegter-include-job-meta-in-base-labels branch from 4fbcab3 to 9aa6b0a Compare October 2, 2024 14:29
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

@mvegter
Copy link
Contributor Author

mvegter commented Oct 2, 2024

I believe my push with @aimeeu suggestion just arrived before @tgross approval , so I think all should be included now . Thanks both !

@tgross tgross added this to the 1.9.0 milestone Oct 2, 2024
@tgross tgross merged commit 3ecf0d2 into hashicorp:main Oct 2, 2024
20 checks passed
@tgross
Copy link
Member

tgross commented Oct 2, 2024

Ok, this has been merged and will ship in Nomad 1.9.0. Thanks for the contribution @mvegter!

@mvegter mvegter deleted the mvegter-include-job-meta-in-base-labels branch October 2, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants