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

Discover service names from process env vars #1195

Merged
merged 8 commits into from
Sep 30, 2024
Merged

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Sep 23, 2024

Since we are already reading the proc file system for the instrumented processes, why not read their env variables for supporting OTEL_SERVICE_NAME and OTEL_RESOURCE_ATTRS. This PR parses the process environment variables for the OTEL variable definitions.

TODO:

  • Unit tests
  • Integration tests

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.

Project coverage is 82.00%. Comparing base (0130316) to head (68f61c4).

Files with missing lines Patch % Lines
pkg/internal/exec/procenv.go 78.94% 2 Missing and 2 partials ⚠️
pkg/internal/exec/file.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1195      +/-   ##
==========================================
+ Coverage   73.29%   82.00%   +8.71%     
==========================================
  Files         135      137       +2     
  Lines       11549    11588      +39     
==========================================
+ Hits         8465     9503    +1038     
+ Misses       2427     1553     -874     
+ Partials      657      532     -125     
Flag Coverage Δ
integration-test 59.84% <81.57%> (+0.13%) ⬆️
k8s-integration-test 59.11% <71.05%> (+0.15%) ⬆️
oats-test 35.73% <71.05%> (+0.08%) ⬆️
unittests 53.20% <63.15%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@mariomac mariomac left a comment

Choose a reason for hiding this comment

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

Nice addition! I guess it's useful for automatic instrumentation of applications that already ship the OTEL SDK, but disabled. Any other concrete use case?

}
}

fmt.Printf("%v\n", vars)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you forgot to remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, nice catch!!!

@grcevski
Copy link
Contributor Author

Nice addition! I guess it's useful for automatic instrumentation of applications that already ship the OTEL SDK, but disabled. Any other concrete use case?

Yeah exactly, well technically we support this usage in sidecar mode right? We can pass to Beyla the OTEL env vars and it will pick them up. So I thought, why not support this in daemonset mode instead of forcing people to use our Beyla config files to name their services. I think it's more efficient to use the OTEL standard, if folks have configured their applications using the OTEL variables we can pick them up.

It's mainly targeted at people that want to rename their services for the purpose of reporting. If I wanted to do this today, I'd have to add Beyla discovery configuration section. Which means that if Beyla is monitoring all services on a node, I have to restart the daemonset if I wanted to add new service definition. By using the OTEL variables we can keep Beyla running, not change any config and we'll automatically pick up their desired name.

At the same time, this approach allows us to pass custom resource attributes per service. We don't have this option today in our config.

Copy link
Contributor

@mariomac mariomac left a comment

Choose a reason for hiding this comment

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

Great addition!!

@grcevski grcevski merged commit 47b960e into grafana:main Sep 30, 2024
10 checks passed
@grcevski grcevski deleted the env_vars branch September 30, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants