-
Notifications
You must be signed in to change notification settings - Fork 131
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a ./gradlew :nessie-quarkus:quarkusBuild
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here)
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confess I don't know, I am going to run a few tests now to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍 thanks for the detailed explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - if @dimas-b is happy, too
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaulting to quarkus.application.name
SGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I made the change. You will need to re-approve, sorry.
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 likequarkus.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:
helm/nessie/README.md
to explain how to test Nessie with OpenTelemetry enabled, and with custom Docker images;site/docs/try/configuration.md
to create a new Traces chapter with a few troubleshooting tips.Fixes #6105