Helm logging support (#1) #38

rockb1017 · 2021-03-13T06:03:49Z

Improving the helm chart:

k8s tagger to enrich with k8s metadata from container logs
remove hostport from default values.yaml
Optimize container log parsing by specifying the container runtime
make it configurable to collect otel collector log
create cluster role and role binding required for k8s metadata enrichment

linux-foundation-easycla · 2021-03-13T06:03:52Z

The committers are authorized under a signed CLA.

✅ Rock Baek (24f0576)

charts/opentelemetry-collector/templates/_config.tpl

tigrannajaryan · 2021-03-17T13:38:44Z

@rockb1017 please sign the CLA.

rockb1017 · 2021-03-17T23:15:03Z

@rockb1017 please sign the CLA.

Do I do a individual CLA?

tigrannajaryan · 2021-04-07T13:35:34Z

@rockb1017 is this PR still needed?

rockb1017 · 2021-04-09T01:30:46Z

@tigrannajaryan
I rebased and updated my commit.
It includes

k8s tagger to enrich with k8s metadata from container logs
remove hostport from default values.yaml
Optimize container log parsing by specifying the container runtime
make it configurable to collect otel collector log
create cluster role and role binding required for k8s metadata enrichment
disable liveness and readiness because it is not working atm

charts/opentelemetry-collector/templates/clusterRole.yaml

charts/opentelemetry-collector/templates/clusterRoleBinding.yaml

tigrannajaryan · 2021-04-09T15:58:17Z

EasyCLA is stuck. Let me try to close and reopen.

tigrannajaryan · 2021-04-09T15:58:59Z

@rockb1017 please update PR description / commit message to reflect what you listed here #38 (comment)

naseemkullah · 2021-04-09T16:02:34Z

charts/opentelemetry-collector/values.yaml

@@ -110,25 +110,21 @@ ports:
    enabled: true
    containerPort: 4317
    servicePort: 4317
-    hostPort: 4317


Can you please keep the hostPorts enabled conditionally if being run as a daemonset (agent) ?

@naseemkullah Could you explain why you would want it by default?
without using hostPort, it can be accessed through using k8s service.
in my AWS EKS cluster, it fails to be deployed when trying to use hostPort. it seems that it requires additional cluster configuration to map it to host port. It would make it a bit more complicated to deploy it.

This how the instrumented workloads communicate with the collector when it runs as a daemon set in agent mode, I agree that in standalone collector mode they should be disabled and the k8s service used, hence enabling them under the condition that agent mode is enabled.

so pods that are running apps that has been instrumented to send trace information are sending it to its node ip it is running on?

thats right! with something like this in their env:

- name: OTEL_EXPORTER_JAEGER_AGENT_HOST valueFrom: fieldRef: fieldPath: status.hostIP

the alternative is running the otel-collector agent as a sidecar container, which is outside the scope of this helm chart

I agree, let's keep host ports. The only stable pipeline of the otel collector currently is tracing. And running agent with no ports exposed doesn't provide any value for traces collection.

in my AWS EKS cluster, it fails to be deployed when trying to use hostPort. it seems that it requires additional cluster configuration to map it to host port. It would make it a bit more complicated to deploy it.

@rockb1017 Could you please share the errors you see? I never saw any issues with exposed ports on this deamonset.

charts/opentelemetry-collector/templates/_pod.tpl

remove hostport from default values.yaml Optimize container log parsing by specifying the container runtime make it configurable to collect otel collector log create cluster role and role binding required for k8s metadata enrichment

dmitryax · 2021-04-13T04:27:18Z

charts/opentelemetry-collector/values.yaml

+    includeAgentLogs: false
+    enrichK8sMetadata: true
+    listOfAnnotations: []
+    listOfLabels: []


K8s metadata applicable not only to logs, but widely used for traces and metrics as well.

All the default configuration can be easily overridden (by merging with defaults) in user's values.yaml. So I don't see a lot of benefit in providing extra configuration ways. Especially listOfAnnotations and listOfLabels can be easily replaced by the existing configuration method.

config: processors: k8s_tagger: annotations: ... labels: ...

it's just one line more to print than

agentCollector: containerLogs: listOfAnnotations: listOfLabels:

but less configuration options -> less confusion.

Speaking of enrichK8sMetadata, I would recommend to move the option one level up to be able to apply it to any of the pipelines. Like:

agentCollector: pipelinesWithK8sMetadata: [metrics, traces, logs]

Let's keep agentCollector.k8sMetadataPipelines: [] by default.

@tigrannajaryan @naseemkullah thoughts?

K8s metadata applicable not only to logs, but widely used for traces and metrics as well.

All the default configuration can be easily overridden (by merging with defaults) in user's values.yaml. So I don't see a lot of benefit in providing extra configuration ways. Especially listOfAnnotations and listOfLabels can be easily replaced by the existing configuration method.

config: processors: k8s_tagger: annotations: ... labels: ...

it's just one line more to print than

agentCollector: containerLogs: listOfAnnotations: listOfLabels:

but less configuration options -> less confusion.

Good catch 💯

Speaking of enrichK8sMetadata, I would recommend to move the option one level up to be able to apply it to any of the pipelines. Like:

agentCollector: pipelinesWithK8sMetadata: [metrics, traces, logs]

Let's keep agentCollector.k8sMetadataPipelines: [] by default.

I just learned about this. so we would have both agentCollector.pipelines and agentCollector.pipelinesWithK8sMetadata?

Seeing that this is a helm chart maybe we just add the k8s_tagger and so on directly in the pipelines out of the box if that makes sense?

I just learned about this. so we would have both agentCollector.pipelines and agentCollector.pipelinesWithK8sMetadata?

right

Seeing that this is a helm chart maybe we just add the k8s_tagger and so on directly in the pipelines out of the box if that makes sense?

Sounds good, but having k8s_tagger enabled by default requires installing the cluster role. At least we need a way to opt out from that.

but less configuration options -> less confusion.

I agree.

I think by having less config, you are making the users to study more on documentation to properly set up configOverride.
For example,

k8s_tagger: passthrough: false auth_type: "kubeConfig" pod_association: - from: resource_attribute name: k8s.pod.uid extract: metadata: - deployment - cluster - namespace - node - startTime annotations: [] labels: [] filter: node_from_env_var: KUBE_NODE_NAME

this is required config for enriching it with k8s metadata.
most of it is static. Users doesn't need to know what passthrough value they need to pick, nor search though this plugin documentation to properly choose what pod_association I need to use.
So I think in this case, "less configuration options -> less confusion" doesn't quite fit here. it is more like "more guided helm chart -> less confusion".
We are helping users by presenting the configuration they need to look and hiding what they don't need to waste their minds on.

charts/opentelemetry-collector/templates/_pod.tpl

tigrannajaryan · 2021-04-13T15:46:58Z

charts/opentelemetry-collector/templates/_config.tpl

-            expr: '$$record matches "^[^ Z]+ "'
-          - output: parser-containerd
-            expr: '$$record matches "^[^ Z]+Z"'
+      {{- if eq .Values.agentCollector.containerLogs.containerRunTime "cri-o" }}


I do not fully understand what this change does. Did this change delete the ability to automatically detect the format used by kubernetes and requires the user to specify the format?

It looks like that

@rockb1017 please clarify why want to change this. It appears to me automatic detection is valuable since there is one less thing for users to worry about.

yes, instead of collector detecting the format, we are allowing users to provide their container runtime. this is how it is done in SCK. Though I am not saying this is definitely better way, but at least it will definitely be better for performance. I would say asking users to specify the container runtime once and have more efficient pipeline is better.

It is a tradeoff between ease of use and performance. Both are valid approach and different users likely will have different preferences. I think what we can do is by default have automatic detection of the format and in addition to that allow the user to specify the format, in which case the detection will be disabled and we will have better performance.

dmitryax · 2021-06-17T23:05:59Z

Closing this PR since it's too specific for a particular logs collection use case. Feel free to open it again if you want to address the comments

rockb1017 requested review from a team March 13, 2021 06:03

andrzej-stencel mentioned this pull request Mar 15, 2021

Enable k8s log collection in Otel Helm Chart. open-telemetry/opentelemetry-collector-contrib#2536

Closed

sumo-drosiek reviewed Mar 17, 2021

View reviewed changes

charts/opentelemetry-collector/templates/_config.tpl Show resolved Hide resolved

tigrannajaryan mentioned this pull request Mar 17, 2021

filelog: Allow placing attributes into Resource instead of LogRecord open-telemetry/opentelemetry-collector-contrib#2725

Closed

techmunk mentioned this pull request Apr 2, 2021

Regex for extract_metadata_from_filepath added by #36 is too strict #42

Closed

rockb1017 force-pushed the helm_logging branch from 30094a5 to 24f0576 Compare April 9, 2021 01:13

sumo-drosiek reviewed Apr 9, 2021

View reviewed changes

charts/opentelemetry-collector/templates/clusterRole.yaml Outdated Show resolved Hide resolved

sumo-drosiek reviewed Apr 9, 2021

View reviewed changes

charts/opentelemetry-collector/templates/clusterRoleBinding.yaml Outdated Show resolved Hide resolved

tigrannajaryan requested a review from dmitryax April 9, 2021 15:57

tigrannajaryan closed this Apr 9, 2021

tigrannajaryan reopened this Apr 9, 2021

naseemkullah reviewed Apr 9, 2021

View reviewed changes

charts/opentelemetry-collector/templates/_pod.tpl Outdated Show resolved Hide resolved

rockb1017 force-pushed the helm_logging branch from a25cea5 to 2d452ee Compare April 10, 2021 05:10

rockb1017 requested review from tigrannajaryan and naseemkullah April 10, 2021 05:10

dmitryax reviewed Apr 13, 2021

View reviewed changes

charts/opentelemetry-collector/templates/_pod.tpl Show resolved Hide resolved

tigrannajaryan reviewed Apr 13, 2021

View reviewed changes

dmitryax closed this Jun 17, 2021

matej-g pushed a commit to matej-g/opentelemetry-helm-charts that referenced this pull request Oct 30, 2023

chore: update collector to 0.86.0 (open-telemetry#38)

3d257b7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helm logging support (#1) #38

Helm logging support (#1) #38

rockb1017 commented Mar 13, 2021 •

edited

Loading

linux-foundation-easycla bot commented Mar 13, 2021 •

edited

Loading

tigrannajaryan commented Mar 17, 2021

rockb1017 commented Mar 17, 2021

tigrannajaryan commented Apr 7, 2021

rockb1017 commented Apr 9, 2021

tigrannajaryan commented Apr 9, 2021

tigrannajaryan commented Apr 9, 2021

naseemkullah Apr 9, 2021

rockb1017 Apr 10, 2021

naseemkullah Apr 10, 2021

rockb1017 Apr 12, 2021

naseemkullah Apr 12, 2021

naseemkullah Apr 12, 2021

dmitryax Apr 13, 2021

dmitryax Apr 13, 2021

dmitryax Apr 13, 2021

naseemkullah Apr 13, 2021

dmitryax Apr 13, 2021

tigrannajaryan Apr 13, 2021

rockb1017 Apr 19, 2021

tigrannajaryan Apr 13, 2021

sumo-drosiek Apr 14, 2021

tigrannajaryan Apr 14, 2021

rockb1017 Apr 19, 2021

tigrannajaryan Apr 19, 2021

dmitryax commented Jun 17, 2021

Helm logging support (#1) #38

Helm logging support (#1) #38

Conversation

rockb1017 commented Mar 13, 2021 • edited Loading

linux-foundation-easycla bot commented Mar 13, 2021 • edited Loading

tigrannajaryan commented Mar 17, 2021

rockb1017 commented Mar 17, 2021

tigrannajaryan commented Apr 7, 2021

rockb1017 commented Apr 9, 2021

tigrannajaryan commented Apr 9, 2021

tigrannajaryan commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitryax commented Jun 17, 2021

rockb1017 commented Mar 13, 2021 •

edited

Loading

linux-foundation-easycla bot commented Mar 13, 2021 •

edited

Loading