-
Notifications
You must be signed in to change notification settings - Fork 260
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
[Resources.Container] Add Kubernetes support #1699
base: main
Are you sure you want to change the base?
Conversation
src/OpenTelemetry.ResourceDetectors.Container/ContainerResourceDetector.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.ResourceDetectors.Container/ContainerResourceDetector.cs
Outdated
Show resolved
Hide resolved
@@ -18,6 +21,13 @@ public class ContainerResourceDetector : IResourceDetector | |||
private const string Filepath = "/proc/self/cgroup"; | |||
private const string FilepathV2 = "/proc/self/mountinfo"; | |||
private const string Hostname = "hostname"; | |||
private const string K8sServiceHostKey = "KUBERNETES_SERVICE_HOST"; | |||
private const string K8sServicePortKey = "KUBERNETES_SERVICE_PORT_HTTPS"; | |||
private const string K8sNamespaceKey = "KUBERNETES_NAMESPACE"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this environment variable and KUBERNETES_CONTAINER_NAME
available in all Kubernetes environments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I know, KUBERNETES_NAMESPACE
and KUBERNETES_CONTAINER_NAME
are not available by default in Kubernetes. The user will have to provide them using the downward API.
var @namespace = Environment.GetEnvironmentVariable(K8sNamespaceKey); | ||
var hostname = Environment.GetEnvironmentVariable(K8sHostnameKey); | ||
var containerName = Environment.GetEnvironmentVariable(K8sContainerNameKey); | ||
var url = $"https://{host}:{port}/api/v1/namespaces/{@namespace}/pods/{hostname}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the guarantee that this will be a well-formed URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing I think but in this case an exception will be thrown, catched and logged using the EventSource.
private const string K8sServicePortKey = "KUBERNETES_SERVICE_PORT_HTTPS"; | ||
private const string K8sNamespaceKey = "KUBERNETES_NAMESPACE"; | ||
private const string K8sHostnameKey = "HOSTNAME"; | ||
private const string K8sContainerNameKey = "KUBERNETES_CONTAINER_NAME"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tricky part here - k8s doesn't have any standard variable to provide container name. It is entirely based on the user. I don't see any issues that extractor would expect it to be named "KUBERNETES_CONTAINER_NAME" - but probably it may be better to be passed by user to detector as constructor argument.
If it is passed by user, we can also report it as additional attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the same for KUBERNETES_NAMESPACE
in this case, which is not standard as well. Perhaps we can let the user supply the values via the constructor but also keep automatic detection via the environment variables if they are not supplied. What do you think?
If it is passed by user, we can also report it as additional attribute.
I'm not sure that's the purpose of this detector. From my point of view, the user can already use the env variable OTEL_RESOURCE_ATTRIBUTES
or implement his own detector to add this resource attribute.
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Not stale. I'll try to finish review this week. |
As this is something that can already be done by collector, as noted in this comment, I think it'd be better to have consensus on this approach in broader opentelemetry community, as suggested by @Kielek, before merging this PR. |
If I read k8s processor code correct, it in fact set container id if it was not set. For it, AttributeK8SContainerName should be set. Same is documented. |
I have an idea how we can finish this PR and still be in full compliance with already approved semantic conventions and idea behind k8s processor implementation. Though it will have one small hack (that can be improved later once proper API will be provided). K8s processor imply that user will provide
Now, instead of accessing some environment variables, we just need to get attributes from internal static class K8sSemanticConventions
{
public const string AttributePodName = "k8s.pod.name";
public const string AttributeContainerName = "k8s.container.name";
}
private string? ExtractK8sContainerId()
{
// Is there any way to do it better?
var environmentVariableAttributes = ResourceBuilder.CreateEmpty()
.AddEnvironmentVariableDetector().Build().Attributes;
var podName = environmentVariableAttributes
.FirstOrDefault(atr => atr.Key == K8sSemanticConventions.AttributeContainerName)
.Value?.ToString();
var containerName = environmentVariableAttributes
.FirstOrDefault(atr => atr.Key == K8sSemanticConventions.AttributePodName)
.Value?.ToString();
...
} |
With this approach, the pod name and container name will be part of the Resource (as it's quite common to enable the env variable detector) and will be included in all traces/metrics/logs, which is not necessarily the desired behavior. 😕 |
@joegoldman2, currently I see several options:
Or some combination of that approaches. I don't like last option (do nothing). |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
@iskiselev I'm not really sure what to do next? Do you want to initiate a new discussion in opentelemetry-specification about new environment variables? |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Not stale. Waiting for @iskiselev's feedback. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Not stale. open-telemetry/opentelemetry-specification#4140 opened to discuss next step. |
Ok, so I guess the next step is to define the desired API to allow users to set the values programmatically, right? |
@joegoldman2, I already suggested API at #1699 (comment), but any would works once it allows to specify values. |
And looks like new environment variable would not be added any time soon: open-telemetry/opentelemetry-specification#2891 (comment). So, using API-based values may be the only option. |
Thank you for the API proposal #1699 (comment). What is the procedure to review/validate a new API? |
Fixes #1562.
Only tested manually at the moment.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes