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

[autoinstrumentation] Add node and pod ip env vars automatically #2769

Merged

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Mar 19, 2024

Description:
Add node IP and pod IP to env vars automatically. This makes them easy to reference in language-specific env vars if needed. Most of the changes are updates to tests.

Link to tracking Issue(s):

Related to #2180

Testing:
Unit and e2e tests

Documentation:

Updated the readme

@TylerHelmuth TylerHelmuth marked this pull request as ready for review March 19, 2024 18:21
@TylerHelmuth TylerHelmuth requested a review from a team March 19, 2024 18:21
@@ -33,7 +33,9 @@ const (

EnvPodName = "OTEL_RESOURCE_ATTRIBUTES_POD_NAME"
EnvPodUID = "OTEL_RESOURCE_ATTRIBUTES_POD_UID"
EnvPodIP = "OTEL_POD_IP"
Copy link
Member

Choose a reason for hiding this comment

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

is there an attribute name in the spec for the POD and NODE IPs?

we should consider putting these under OTEL_RESOURCE_ATTRIBUTES_

Copy link
Member Author

Choose a reason for hiding this comment

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

I opted not to use OTEL_RESOURCE_ATTRIBUTES_ because I wasn't adding these values to the resource attributes. They are being added so that in Instrumentation users can to http://$(OTEL_NODE_IP):4318 or http://$(OTEL_POD_IP):4318 for their endpoints without having to specify any extra env vars.

Copy link
Member Author

Choose a reason for hiding this comment

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

For semantic convention, there isn't a k8s.pod.ip or k8s.node.ip convention yet.

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM,

I would add the env vars to the changelog as well

component: instrumentation

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add node and pod ips as env vars automatically
Copy link
Member

Choose a reason for hiding this comment

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

Could we list the env vars here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@jaronoff97 jaronoff97 merged commit d672bc9 into open-telemetry:main Apr 2, 2024
31 checks passed
@TylerHelmuth TylerHelmuth deleted the include-node-ip-in-env-vars branch April 2, 2024 20:10
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…n-telemetry#2769)

* Add node and pod ip env vars automatically

* Fix tests

* Fix tests

* Fix tests

* Fix tests

* Fix tests

* Update changelog
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.

4 participants