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

Add example with tracing + opentelemetry exporting #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jaronsummers
Copy link

I don't have a vanilla kubernetes cluster to test on, so I can't say for sure if this works, but I think it is pretty close!

Please let me know if there are any bits that bear more explanation, or anything that you think is unnecessary or poorly configured.

I'm also happy to rearrange the directory any way you'd like, I didn't really have much of an opinion on the best way to lay things out.

@mickael-carl
Copy link
Member

Since you mention not having a vanilla cluster to test, could you try to run this on top of Kind? This should be as easy as (assuming macOS):

brew install kind
kind create cluster

spec:
initContainers:
- name: volume-init
image: "{{ $.Values.global.docker.registry }}/busybox:1.29.3"
Copy link
Member

Choose a reason for hiding this comment

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

That docker registry value seems to be missing:

Error: INSTALLATION FAILED: template: buildbarn/templates/storage.yaml:37:20: executing "buildbarn/templates/storage.yaml" at <$.Values.global.docker.registry>: nil pointer evaluating interface {}.docker

Copy link
Author

Choose a reason for hiding this comment

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

ah yeah, in my configs a bunch of globals get populated ~magically.

will rearrange.

template:
metadata:
annotations:
checksum/common.config: {{ include (print $.Template.BasePath "/config/collector.yaml.tpl") . | sha256sum }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
checksum/common.config: {{ include (print $.Template.BasePath "/config/collector.yaml.tpl") . | sha256sum }}
checksum/common.config: {{ include (print $.Template.BasePath "/config/collector.yaml") . | sha256sum }}

app.kubernetes.io/name: {{ $.Values.baseName }}
app.kubernetes.io/component: opentelemetry-collector
service: buildbarn
team: {{ $.Values.team }}
Copy link
Member

Choose a reason for hiding this comment

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

It appears this value doesn't exist:

Error: INSTALLATION FAILED: unable to build kubernetes objects from release manifest: error validating "": error validating data: unknown object type "nil" in Deployment.metadata.labels.team

- Datadog prometheus scraping configuration: https://docs.datadoghq.com/agent/kubernetes/prometheus/
- Datadog span retention: https://docs.datadoghq.com/tracing/trace_retention_and_ingestion

## Usage Notes
Copy link
Member

Choose a reason for hiding this comment

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

Could you document real quick how to set this up? See for instance how the existing Kubernetes deployment is documented.

kind: Service
metadata:
name: {{ $.Values.baseName }}-opentelemetry-collector
namespace: {{ $.Release.Namespace }}
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, shouldn't this be:

Suggested change
namespace: {{ $.Release.Namespace }}
namespace: {{ $.Values.namespace }}

@@ -0,0 +1,61 @@
baseName: buildbarn
serviceDnsName: svc.mycompany.fake
namespace: buildbarn
Copy link
Member

Choose a reason for hiding this comment

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

This is making the assumption that this namespace already exists, we should probably document that or make sure the namespace is created?

Copy link
Member

Choose a reason for hiding this comment

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

Or preferably: could we merge this with the existing deployment? That way we won't have to maintain 2 different deployments (we already have often outdated examples with 3, adding a 4th is bound to make things worse in some way 😄)

Copy link
Author

Choose a reason for hiding this comment

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

I am very empathetic to the problem of the deployments lagging behind, though I do think there is value in having multiple examples present, since the configs are quite difficult to grok intuitively, at least in my experience.

I'll add a section to create the namespace, and also parameterize it.

"metrics": ["*"]
}
]
labels:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
labels:
labels:
app.kubernetes.io/name: {{ $.Values.baseName }}

This label is missing, while being present in the selector:

Error: INSTALLATION FAILED: Deployment.apps "buildbarn-opentelemetry-collector" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"app.kubernetes.io/component":"opentelemetry-collector"}: selector does not match template labels

template:
metadata:
labels:
app.kubernetes.io/component: frontend
Copy link
Member

Choose a reason for hiding this comment

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

Same here, labels don't match the selector

}
]
labels:
app.kubernetes.io/component: storage
Copy link
Member

Choose a reason for hiding this comment

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

Missing label here as well

traces:
receivers: [otlp]
processors: [batch, k8sattributes]
exporters: [datadog]
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be invalid?

Error: cannot load configuration: unknown exporters type "datadog" for datadog
2021/12/28 01:08:34 collector server run finished with error: cannot load configuration: unknown exporters type "datadog" for datadog

Copy link
Author

Choose a reason for hiding this comment

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

huh, interesting. I didn't even modify the collector config, I just copy and pasted it directly from my deployment, which is definitely working.


## Requirements:

1. Datadog agent already deployed in the kubernetes cluster: https://docs.datadoghq.com/agent/kubernetes/?tab=helm#installation
Copy link
Member

Choose a reason for hiding this comment

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

Could this be set as a dependency of this chart?

@jaronsummers
Copy link
Author

Since you mention not having a vanilla cluster to test, could you try to run this on top of Kind? This should be as easy as (assuming macOS):

brew install kind

kind create cluster

I am on holiday, will give this a try when I get home, and will also review and respond to the rest of your feedback.

Thanks for taking a look!

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.

2 participants