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

Fix monitoring of collector when in daemonset mode #72

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

bismarck
Copy link
Contributor

Description

  • Fixed monitoring of collector when in daemonset mode. This was broken when environment variables were renamed in b6354b1
  • Use OTEL_K8S_POD_IP in otel-collector job so that instance label can be unique.

How Has This Been Tested?

Please describe how you tested your changes. When applicable, provide instructions on how the reviewer can also test your changes.


R/CC: Who should know about this change? Requesting any reviewers in particular?

@bismarck
Copy link
Contributor Author

For consistency's sake, I would love to collect metrics from otel collector pods the same way. Currently, the metrics collector and traces collector use a ServiceMonitor and the logs collector uses this static config job. Any reason why everything can't use a ServiceMonitor?

@jaronoff97
Copy link
Collaborator

jaronoff97 commented Feb 29, 2024

@bismarck i think that's plenty reasonable. i think the only reason it isn't done that way is because i forgot to add it here. FWIW, i am hoping to iron out a lot of the frustrations with this chart by upstreaming it to the actual opentelemetry helm charts repo where it will get much more attention and care. see open-telemetry/opentelemetry-helm-charts#562. Would you want to do that in addition to this change? Also, you'll need to bump the chart version.

@bismarck
Copy link
Contributor Author

@jaronoff97 I can add the ServiceMonitor for the logs collector. Do you still want to keep the static config job or can we get rid of it?

@jaronoff97
Copy link
Collaborator

let's keep it for now – I think regardless it's a good change :)

@jaronoff97
Copy link
Collaborator

@bismarck running the linter quickly and then ill merge. If you have the time, I'm upstreaming this chart to the main collector repository and would love any feedback you may have

@jaronoff97 jaronoff97 merged commit 2eec010 into lightstep:main Mar 5, 2024
1 check passed
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