LGTM Stack through helm #359
Conversation
| location = /nginx_status { | ||
| stub_status on; | ||
| access_log off; | ||
| # Allow access from private IP ranges used in k8s clusters |
There was a problem hiding this comment.
what is the reason for the ip allowlist? IMO we don't need this? They will keep their service behind a private lb anyway and sometimes nice to be able to fetch metrics and thigns from frontend/lb
There was a problem hiding this comment.
reason was to block access but that makes sense, will keep same as health endpoint
| langSmithReleaseName: "langsmith" # Change this value to match your LangSmith release name. | ||
|
|
||
| # ======================== Metrics Exporters ======================== | ||
| kubestatemetrics: |
There was a problem hiding this comment.
nit, can we change these all to camel case?
There was a problem hiding this comment.
The nginx and redis exporter both use {{ Chart.name }} as the container name, so we can't use camel case because of kubernetes container name restritctions. Switching to redis-exporter, nginx-exporter, etc..
charts/langsmith/values.yaml
Outdated
|
|
||
| observability: | ||
| # This will add an annotation to your containers telling the OTEL Operator to bring up a sidecar container. Used in the LangSmith Observability helm chart. | ||
| sidecarCollector: |
There was a problem hiding this comment.
perhaps we can just use commonAnnotations for this? (so in the guide just direct user to add this explicit annotation to their commonAnnotation section?
There was a problem hiding this comment.
The annotations need to be under the spec.template.metadata.annotations section, which we don't use commonAnnotations for atm. Fine to add commonAnnotations to those annotations?
charts/langsmith-observability/templates/otel/rbac-gateway.yaml
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,108 @@ | |||
| {{- if or .Values.otelCollector.metrics.enabled .Values.otelCollector.traces.enabled }} | |||
There was a problem hiding this comment.
is there a world where we have justs the gateway/no sidecars? We don't actually run that many services insside langsmith so might be fine to exclude sidecars for perf reassons?
There was a problem hiding this comment.
Discussed offline, sticking with sidecar because of logs on filesystem
There was a problem hiding this comment.
Removed the s3 configurations since these use a different helm chart for mimir and tempo - hence we don't support at all
There was a problem hiding this comment.
No Mimir chart for the single binary version, only distributed.
builds on top of langchain-ai/helm#359
This branch builds on top of this PR: #353