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

Remove the need to set environment variables with hostmetricsreceiver #23861

Merged
merged 8 commits into from
Jul 25, 2023
Merged

Remove the need to set environment variables with hostmetricsreceiver #23861

merged 8 commits into from
Jul 25, 2023

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jun 30, 2023 •
edited
Loading

Description:
Remove the need to set environment variables with hostmetricsreceiver

Testing:
Tests unchanged.

Documentation:
No docs touched.

@atoulme atoulme requested a review from a team June 30, 2023 07:09
@atoulme atoulme requested a review from dmitryax as a code owner June 30, 2023 07:09
@github-actions github-actions bot added cmd/configschema configschema command cmd/otelcontribcol otelcontribcol command exporter/datadog Datadog components labels Jun 30, 2023
@atoulme atoulme force-pushed the hostmetricsreceiver_env branch 6 times, most recently from d789620 to 1eaddfc Compare June 30, 2023 08:05
mx-psi
mx-psi reviewed Jun 30, 2023
View reviewed changes
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Can we wait to merge this until shirou/gopsutil/pull/1439 is in a tagged release of gopsutil?

frzifus
frzifus reviewed Jun 30, 2023
View reviewed changes
Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

Hi @atoulme, I'm not sure I fully get the context, could you explain it briefly? ^^

mx-psi
mx-psi reviewed Jul 5, 2023
View reviewed changes
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Looks like 3.23.6 includes your change @atoulme 🚀 , can you update this PR?

@atoulme
Copy link
Contributor Author

atoulme commented Jul 7, 2023

Hi @atoulme, I'm not sure I fully get the context, could you explain it briefly? ^^

The hostmetricsreceiver uses gopsutil which traditionally has allowed configuration via environment variables. This messes with our internals because those env vars are not thread safe and we use those in multiple places. For example, here. In general, the program should never set environment variables.

frzifus
frzifus approved these changes Jul 7, 2023
View reviewed changes
Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

Ahh I got it, thanks for explanation @atoulme !

@atoulme atoulme force-pushed the hostmetricsreceiver_env branch 2 times, most recently from c3e6f6b to 7174dfd Compare July 8, 2023 06:37
@atoulme atoulme force-pushed the hostmetricsreceiver_env branch 3 times, most recently from ddf081c to b1d4050 Compare July 8, 2023 08:03
@atoulme
Copy link
Contributor Author

atoulme commented Jul 11, 2023

@dmitryax as codeowner, please take a look when you get a chance, thanks!

@atoulme atoulme force-pushed the hostmetricsreceiver_env branch 2 times, most recently from 5f9c61b to 6b662cb Compare July 24, 2023 06:37
@dmitryax dmitryax merged commit 36fc426 into open-telemetry:main Jul 25, 2023
90 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 25, 2023
@atoulme atoulme deleted the hostmetricsreceiver_env branch July 25, 2023 22:52
@crobert-1 crobert-1 mentioned this pull request Sep 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mx-psi mx-psi mx-psi approved these changes

@dmitryax dmitryax dmitryax approved these changes

@frzifus frzifus frzifus approved these changes

@songy23 songy23 Awaiting requested review from songy23

@dineshg13 dineshg13 Awaiting requested review from dineshg13

@gbbr gbbr Awaiting requested review from gbbr

@liustanley liustanley Awaiting requested review from liustanley

@pmcollins pmcollins Awaiting requested review from pmcollins

Assignees

@TylerHelmuth TylerHelmuth

Labels
cmd/configschema configschema command cmd/otelcontribcol otelcontribcol command exporter/datadog Datadog components receiver/hostmetrics
Projects
None yet
Milestone
v0.82.0
Development

Successfully merging this pull request may close these issues.
5 participants
@atoulme @mx-psi @dmitryax @frzifus @TylerHelmuth