Redesign telemetry support in Helm chart #6153
Conversation
This commit removes the old jaegerTracing section from the values.yaml file. Indeed, OpenTracing support has been replaced in 0.46.0 with OpenTelemetry support, and properties like quarkus.jaeger.service.name are not honored anymore. Instead, this commit creates a new tracing section that maps to OpenTelemetry configuration properties. This commit also introduces some doc changes: - In helm/nessie/README.md to explain how to test Nessie with OpenTelemetry enabled, and with custom Docker images; - In site/docs/try/configuration.md to create a new Traces chapter with a few troubleshooting tips.
| {{- end }} | ||
| {{- if .Values.tracing.sampler }} | ||
| - name: QUARKUS_OPENTELEMETRY_TRACER_SAMPLER | ||
| value: {{ .Values.tracing.sampler | toString | replace "true" "on" | replace "false" "off" | quote }} |
There was a problem hiding this comment.
The calls to replace are required because in Yaml, on and off are actually booleans, but the server expects the strings on or off, not true or false.
Codecov ReportBase: 77.36% // Head: 83.78% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #6153 +/- ##
============================================
+ Coverage 77.36% 83.78% +6.41%
============================================
Files 565 49 -516
Lines 17260 1819 -15441
Branches 1723 328 -1395
============================================
- Hits 13353 1524 -11829
+ Misses 3282 216 -3066
+ Partials 625 79 -546
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
|
||
| ```bash | ||
| eval $(minikube docker-env) | ||
| docker build -f ./tools/dockerbuild/docker/Dockerfile-jvm -t nessie-test:latest ./servers/quarkus-server |
There was a problem hiding this comment.
Maybe add a ./gradlew :nessie-quarkus:quarkusBuild ?
There was a problem hiding this comment.
Nice to see that that these Dockerfiles help w/ k8s stuff as well.
|
|
||
| ```bash | ||
| eval $(minikube docker-env) | ||
| docker build -f ./tools/dockerbuild/docker/Dockerfile-jvm -t nessie-test:latest ./servers/quarkus-server |
| @@ -1,5 +1,5 @@ | |||
| # -- The number of replicas to deploy (horizontal scaling). | |||
| # Beware that replicas are stateless; don't set this number > 1 when using ROCKS version store type. | |||
| # Beware that replicas are stateless; don't set this number > 1 when using INMEMORY or ROCKS version store types. | |||
servers/quarkus-server/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
| ```bash | ||
| helm install nessie -n nessie-ns helm/nessie \ | ||
| --set tracing.enabled=true \ | ||
| --set tracing.endpoint=http://jaeger-collector:4317 |
There was a problem hiding this comment.
Would it work with the otlp/2 protocol (default port 55680)?.. or do we need to configure something else for otlp/2 perhaps?
There was a problem hiding this comment.
I confess I don't know, I am going to run a few tests now to investigate.
There was a problem hiding this comment.
Here is what I found:
According to this issue: open-telemetry/opentelemetry-specification#1148, it seems that in its early stages, OpenTelemetry chose to use ports 55680 and 55681 for OTLP/gRPC and OTLP/HTTP respectively.
However, as the discussion in the issue reveals, these choices weren't good ones, because these ports are in the ephemeral range, and consequently, are not suitable to be used as service ports.
So, they asked IANA to reserve instead two other ports: 4317 and 4318, the common ports that we use today for OTLP/gRPC and OTLP/HTTP respectively.
If you look at the specs in 2020, the default endpoint for OTLP/gRPC was localhost:55680.
But if you look at the specs now, the default endpoint for OTLP/gRPC was changed to http://localhost:4317.
As a conclusion, I don't think there is any other alternate port that we should be supporting, only 4317 is nowadays being used for OTLP/gRPC.
There was a problem hiding this comment.
nice 👍 thanks for the detailed explanation!
helm/nessie/values.yaml
Outdated
| sample: all | ||
| # -- Resource attributes to identify the nessie service among other tracing sources. | ||
| # See https://opentelemetry.io/docs/reference/specification/resource/semantic_conventions/#service. | ||
| # If left empty, the following attribute will be automatically added: service.name=nessie. |
There was a problem hiding this comment.
I am having second thoughts about this.
If service.name is not provided at all, it seems that Quarkus will use the value from quarkus.application.name, which in our case is Nessie. This might be enough, wdyt?
There was a problem hiding this comment.
Defaulting to quarkus.application.name SGTM.
There was a problem hiding this comment.
OK I made the change. You will need to re-approve, sorry.
This commit removes the old
jaegerTracingsection from the values.yaml file. Indeed, OpenTracing support has been replaced in 0.46.0 with OpenTelemetry support, and properties likequarkus.jaeger.service.nameare not honored anymore.Instead, this commit creates a new
tracingsection that maps to OpenTelemetry configuration properties.This commit also introduces some doc changes:
helm/nessie/README.mdto explain how to test Nessie with OpenTelemetry enabled, and with custom Docker images;site/docs/try/configuration.mdto create a new Traces chapter with a few troubleshooting tips.Fixes #6105