fix(logging): Update otel and tracing config #5291

lc525 · 2024-02-05T13:09:11Z

What this PR does / why we need it:

fixes dataflow engine logging errors by specifying the otel exporter protocol

the updates are needed because the opentelemetry-java-instrumentation library requires a http(s) URI for the otel collector endpoint, regardless of the actual protocol. As the default for us is grpc, explicitly set the OTEL_EXPORTER_OTLP_PROTOCOL environment variable on dataflow pods

fixes otel-collector config to remove deprecated jagger exporter (jagger now supports otel directly).
add the OTEL_EXPORTED_OTLP_PROTOCOL key to the seldon-tracing configMap and update the operator, crds and helm charts to support getting the value for this key from tracing config, similarly to how OTEL_EXPORTER_OTLP_PROTOCOL is fetched
update versions used by ansible for jagger and opentelemetry-operator
port 14250 no longer needs to be exposed under any config
fix dependency ordering for dataflow/gradle.
- previous ordering caused kafka-streams not to be able to find the slf4j logging provider
- this lead to logs produced by kafka-streams not being recorded

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #
Internal issue references:

#INFRA-568 Jagger latest is crashing
#INFRA-464 Otel is not able to parse its config (deprecated exporters)

Public issues:

Special notes for your reviewer:

helm chart updates generated via makefile; in order to ensure results consistent to previous helm charts (i.e descriptions that don't disappear), I had to add some docs / update the makefile; those changes are unrelated to the primary goal of the PR.
k8s/yamls/* - auto generated via makefile

lc525 · 2024-02-05T13:48:34Z

k8s/helm-charts/seldon-core-v2-runtime/templates/seldon-runtime.yaml

@@ -55,4 +55,5 @@ spec:
    tracingConfig:
      disable: {{ .Values.config.tracingConfig.disable }}
      otelExporterEndpoint: {{ .Values.config.tracingConfig.otelExporterEndpoint }}
+      otelExporterProtocol: {{ .Values.config.tracingConfig.otelExporterProtocol }}


@RafalSkolasinski following this change, should we update any of the helm chart versions? I don't know how helm chart version updates are handled, or if there is a policy on updating them. Atm, for example, I've seen that the appVersion has nothing to do with the Seldon Core version. Happy to follow what you suggest here.

lc525 · 2024-02-05T13:51:17Z

operator/config/crd/bases/mlops.seldon.io_pipelines.yaml

The descriptions I've added here were in the current helm-charts but not in the sources that generate them via kustomize (i.e. this file). I've added them because otherwise those descriptions would have disappeared on re-generation via the Makefile.

lc525 · 2024-02-05T13:52:16Z

scheduler/otel-collector-config.yaml

@@ -9,14 +9,14 @@ exporters:
    const_labels:
      provider: seldon

-  logging:
+  debug:


the logger exporter is deprecated and replaced by debug

sakoush

lgtm. left minor comments / questions.

sakoush · 2024-02-05T15:08:19Z

k8s/Makefile

@@ -25,7 +25,7 @@ create-helm-charts:

 .PHONY: create-yaml
 create-yaml:
-	helm template seldon-core-v2-certs ./helm-charts/seldon-core-v2-certs | grep -v "namespace:" > yaml/certs.yaml
+	helm template -n seldon-mesh seldon-core-v2-certs ./helm-charts/seldon-core-v2-certs | grep -v "namespace:" > yaml/certs.yaml


do we want to fix it to seldon-mesh?

I don't know. However, the certs.yaml file currently generated from the helm chart has seldon-mesh, so not adding the namespace here would have changed lots of dnsNames from seldon-mesh to default on re-generation. I've simply added this here to avoid any changes to the generated certs.yaml

Perhaps @RafalSkolasinski could suggest a better way of dealing with this? (i.e. even it involves changing the current certs.yaml)

Note the grep -v cmd to exclude the namespace from the crds as an attempt to make it generic. However I have not tested a deployment with these yaml manifests in any other namespace. I have not realised before that we have things such as dns settings with seldon-mesh hardcoded inside.

Yep, I saw that; It's just that in the case of certs there's never any "namespace:" reference, but the template contains lots of {{ .Release.Namespace }} references within the dnsNames lists, and that gets replaced with either default or whatever gets passed to helm template -n when generating yamls.

Technically, anybody could generate a valid certs.yaml for their own namespace name by running helm template with the right -n [namespace]. Not sure if there's a way of making the yaml generic here. Also, this is hardcoded only at the resulting yaml level, not in terms of the helm chart.

Ok. I wonder then if we should fix the namespace in all yaml manifests?

So, I've checked. In all the other ones {{ .Release.Namespace }} only appears as a value of the namespace key, which is removed by the grep -v, so the resulting yamls do not end up containing any hardcoded values. In that sense they are generic.

k8s/helm-charts/seldon-core-v2-crds/templates/seldon-v2-crds.yaml

lc525 · 2024-02-06T10:10:08Z

k8s/yaml/crds.yaml

file generated from helm charts via Makefile (make create-yaml)

- fixes dataflow engine logging errors by specifying the otel exporter protocol this is required because the `opentelemetry-java-instrumentation` library requires a http(s) URI for the otel collector endpoint, regardless of the actual protocol. As the default for us is grpc, explicitly set the OTEL_EXPORTER_OTLP_PROTOCOL environment variable on dataflow pods - add the OTEL_EXPORTED_OTLP_PROTOCOL key to the seldon-tracing configMap and update the operator, crds and helm charts to support getting the value for this key from tracing config, similarly to how OTEL_EXPORTER_OTLP_PROTOCOL is fetched - update configs for when running outside k8s to match those in k8s

The order in which dependencies are specified for Gradle matters, influencing the ordering in the classpath of the application. Here, the slf4j klogging plugin was introduced after the kafka streams dependency, leading to errors at runtime, with the kafka-streams logging facility not able to find a slf4j provider.

kustomize bases and crds now obtained via running `make manifests` in the operator dir

lc525 requested review from sakoush and RafalSkolasinski as code owners February 5, 2024 13:09

lc525 commented Feb 5, 2024

View reviewed changes

sakoush approved these changes Feb 5, 2024

View reviewed changes

lc525 commented Feb 6, 2024

View reviewed changes

k8s/yaml/crds.yaml Outdated

Copy link

Member Author

lc525 Feb 6, 2024

Choose a reason for hiding this comment

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

file generated from helm charts via Makefile (make create-yaml)

lc525 mentioned this pull request Feb 8, 2024

feat(operator): Expose PodSpec in OverrideSpec for SeldonRuntime #5296

Merged

lc525 added the v2 label Feb 9, 2024

lc525 force-pushed the otel-updates branch from 9aa7369 to eb06ff5 Compare February 9, 2024 13:43

lc525 added 4 commits February 9, 2024 23:14

update(k8s/Makefile): get namespace for certs chart from variable

51f7d07

fix(crds) generate from operator apis instead of manually

9f244b0

kustomize bases and crds now obtained via running `make manifests` in the operator dir

lc525 force-pushed the otel-updates branch from eb06ff5 to 9f244b0 Compare February 9, 2024 23:17

lc525 merged commit b24107e into SeldonIO:v2 Feb 10, 2024
3 of 4 checks passed

lc525 deleted the otel-updates branch February 10, 2024 00:41

This was referenced Feb 10, 2024

otel-collector-1 container not starting: error decoding 'exporters': unknown type: "jaeger" for id: "jaeger" #5189

Closed

fix(scheduler): update otel-collector-config after jaeger endpoint does not supported #5170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(logging): Update otel and tracing config #5291

fix(logging): Update otel and tracing config #5291

lc525 commented Feb 5, 2024 •

edited

Loading

lc525 Feb 5, 2024

lc525 Feb 5, 2024

lc525 Feb 5, 2024

sakoush left a comment •

edited

Loading

sakoush Feb 5, 2024

lc525 Feb 5, 2024 •

edited

Loading

sakoush Feb 5, 2024

lc525 Feb 5, 2024 •

edited

Loading

sakoush Feb 5, 2024

lc525 Feb 6, 2024

lc525 Feb 6, 2024

fix(logging): Update otel and tracing config #5291

fix(logging): Update otel and tracing config #5291

Conversation

lc525 commented Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakoush left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lc525 Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lc525 Feb 5, 2024 • edited Loading

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

lc525 commented Feb 5, 2024 •

edited

Loading

sakoush left a comment •

edited

Loading

lc525 Feb 5, 2024 •

edited

Loading

lc525 Feb 5, 2024 •

edited

Loading