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

[service/proctelemetry] Offer to override the HOST_PROC environment variable with a programmatic value #7998

Merged
merged 14 commits into from
Aug 15, 2023

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jun 28, 2023

Reprising #7434

Description:

While debugging the below error in k8s env

Error: failed to register process metrics: process does not exist
2023/03/23 03:44:47 main.go:115: application run finished with error: failed to register process metrics: process does not exist

I have noticed that the metric server is calling GOPSUTIL while the HOST_PROC variable is set , this causes gopsutil PidExistsWithContext to retrieve the process from the host instead from the container

func PidExistsWithContext(ctx context.Context, pid int32) (bool, error) {
	if pid <= 0 {
		return false, fmt.Errorf("invalid pid %v", pid)
	}
	proc, err := os.FindProcess(int(pid))
	if err != nil {
		return false, err
	}

	if isMount(common.HostProc()) { // if /<HOST_PROC>/proc exists and is mounted, check if /<HOST_PROC>/proc/<PID> folder exists
		_, err := os.Stat(common.HostProc(strconv.Itoa(int(pid))))
		if os.IsNotExist(err) {
			return false, nil
		}
		return err == nil, err
	}

This PR unsets and resets the host_proc variable and introduces an option to allow the use of host_proc if for whatever reason they need to

Link to tracking Issue:
Fixes #7435

Testing:
unit tests

@atoulme atoulme requested review from a team and djaglowski June 28, 2023 21:06
Copy link
Contributor

@dloucasfx dloucasfx left a comment

Choose a reason for hiding this comment

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

LGTM !!! the tests are failing but not related to the change

service/internal/proctelemetry/process_telemetry.go Outdated Show resolved Hide resolved
service/telemetry/config.go Outdated Show resolved Hide resolved
@atoulme atoulme force-pushed the metric-server-hostproc branch 3 times, most recently from 310eb9a to a2ce366 Compare June 30, 2023 00:02
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.34% ⚠️

Comparison is base (9375d95) 90.82% compared to head (2e3f175) 90.49%.
Report is 38 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7998      +/-   ##
==========================================
- Coverage   90.82%   90.49%   -0.34%     
==========================================
  Files         300      301       +1     
  Lines       15108    15727     +619     
==========================================
+ Hits        13722    14232     +510     
- Misses       1112     1207      +95     
- Partials      274      288      +14     
Files Changed Coverage Δ
...ervice/internal/proctelemetry/process_telemetry.go 85.07% <100.00%> (+1.20%) ⬆️

... and 20 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@atoulme atoulme force-pushed the metric-server-hostproc branch 3 times, most recently from 515d4f9 to a06efc6 Compare June 30, 2023 01:44
@atoulme atoulme force-pushed the metric-server-hostproc branch from a06efc6 to a4e9ff5 Compare July 6, 2023 15:59
.chloggen/metric-server-hostproc.yaml Outdated Show resolved Hide resolved
service/internal/proctelemetry/process_telemetry_test.go Outdated Show resolved Hide resolved
service/telemetry/config.go Outdated Show resolved Hide resolved
@atoulme atoulme changed the title [service/proctelemetry] Unset HOST_PROC to make sure we use and report the actual process and introduce an option to allow user to overwrite the unset logic and use the value set in the environment variable [service/proctelemetry] Offer to override the HOST_PROC environment variable with a programmatic value Jul 20, 2023
@atoulme atoulme force-pushed the metric-server-hostproc branch from 778d9cd to 0147fd5 Compare July 24, 2023 06:35
@atoulme atoulme force-pushed the metric-server-hostproc branch from 0147fd5 to 2ba4d54 Compare July 24, 2023 06:49
@atoulme atoulme added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 1, 2023
@atoulme atoulme closed this Aug 14, 2023
@atoulme atoulme reopened this Aug 14, 2023
@codeboten codeboten merged commit 55902b6 into open-telemetry:main Aug 15, 2023
@atoulme atoulme deleted the metric-server-hostproc branch August 28, 2023 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
4 participants