feat(otel-demo): upgrade OpenSearch 3.x & Jaeger chart 4.x.x , add Spark job, and fix deployment #7843
Conversation
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7843 +/- ##
=======================================
Coverage 95.53% 95.53%
=======================================
Files 307 307
Lines 15911 15911
=======================================
Hits 15201 15201
Misses 558 558
Partials 152 152
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Metrics Comparison SummaryTotal changes across all snapshots: 0 Detailed changes per snapshotsummary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
|
jkowall
left a comment
jkowall
left a comment
There was a problem hiding this comment.
Thanks for the PR, just one note.
| name: jaeger-hotrod | ||
| port: | ||
| number: 80 | ||
| number: 8080 |
There was a problem hiding this comment.
Pull request overview
This pull request upgrades the OTEL demo environment to use OpenSearch 3.x and Jaeger chart 4.x, adds a Spark dependency analysis job, introduces a standalone HotROD deployment, and fixes various configuration issues.
Key Changes:
- Upgraded Jaeger Helm chart to version 4.2.3 and removed deprecated v2 branch cloning logic
- Upgraded OpenSearch and OpenSearch Dashboards to version 3.3.x
- Added Spark dependencies CronJob for trace dependency analysis and standalone HotROD deployment manifest
- Fixed service endpoints and OTLP configurations to use the primary
jaegerservice instead ofjaeger-collector - Split TLS certificates into separate secrets for better certificate management
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| spark-dependencies-cronjob-opensearch.yaml | New CronJob for calculating Jaeger dependencies from OpenSearch storage, runs every 15 minutes |
| otel-demo-values.yaml | Simplified OTEL_RESOURCE_ATTRIBUTES, added frontend service name override, updated collector to use jaeger service |
| opensearch-dashboard-values.yaml | Removed hardcoded image tag to allow version management via deployment script |
| jaeger-values.yaml | Updated OTLP endpoints to jaeger service, standardized image pull policy, configured HotROD service name |
| jaeger-config.yaml | Enabled create_mappings for OpenSearch to ensure proper index setup |
| ingress/ingress-jaeger.yaml | Fixed HotROD service port to 8080, split TLS into separate secrets per host |
| hotrod.yaml | New standalone HotROD deployment with independent lifecycle management |
| deploy-all.sh | Removed deprecated clone logic, added version variables, deployed new components, updated service checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: OTEL_RESOURCE_ATTRIBUTES | ||
| value: service.name.prefix=hotrod- |
There was a problem hiding this comment.
The OTEL_SERVICE_NAME is set to "hotrod-frontend" but there's also a OTEL_RESOURCE_ATTRIBUTES with "service.name.prefix=hotrod-". This configuration is contradictory - the resource attribute appears to be trying to set a service name prefix, but this is not a standard OpenTelemetry attribute. The service.name.prefix attribute doesn't exist in the OpenTelemetry specification. If the intent is to prefix the service name, this should be handled differently or removed.
| - name: OTEL_RESOURCE_ATTRIBUTES | |
| value: service.name.prefix=hotrod- |
| --version "${OPENSEARCH_VERSION}" \ | ||
| --set image.tag="${OPENSEARCH_VERSION}" \ |
There was a problem hiding this comment.
The --version parameter for the Helm chart is being set to the OpenSearch version (3.3.2), but this should be the Helm chart version, not the application version. The chart version and application version are different. You should specify the appropriate chart version separately from the image tag. For example, the chart version might be something like "2.x.x" while the image tag is "3.3.2".
| --version "${OPENSEARCH_DASHBOARDS_VERSION}" \ | ||
| --set image.tag="${OPENSEARCH_DASHBOARDS_VERSION}" \ |
There was a problem hiding this comment.
Similar to the OpenSearch deployment, the --version parameter for the OpenSearch Dashboards Helm chart is being set to the application version (3.3.0), but this should be the Helm chart version, not the application version. The chart version and application version are different and should be specified separately.
| - name: OTEL_EXPORTER_OTLP_ENDPOINT | ||
| value: "http://jaeger:4318" | ||
| - name: OTEL_EXPORTER_OTLP_TRACES_ENDPOINT | ||
| value: "http://jaeger:4318/v1/traces" |
There was a problem hiding this comment.
The HotROD deployment is missing environment variables for OTLP protocol configuration. The jaeger-values.yaml file sets OTEL_EXPORTER_OTLP_PROTOCOL to "http/protobuf" (lines 30-31), but this environment variable is not present in the standalone HotROD deployment. This inconsistency could lead to different behavior between the two deployments.
| value: "http://jaeger:4318/v1/traces" | |
| value: "http://jaeger:4318/v1/traces" | |
| - name: OTEL_EXPORTER_OTLP_PROTOCOL | |
| value: "http/protobuf" |
| memory: 1Gi | ||
| limits: | ||
| cpu: 500m | ||
| memory: 2Gi |
There was a problem hiding this comment.
The memory limit of 2Gi might be insufficient for Spark dependency processing depending on the volume of traces. The JAVA_OPTS sets max heap to 1g, but Spark also needs off-heap memory for overhead. Consider monitoring memory usage and adjusting limits if OOMKilled errors occur. A safer configuration might be to increase the limit to 3Gi or adjust the heap settings to leave more room for off-heap memory.
| memory: 2Gi | |
| memory: 3Gi |
| - hosts: | ||
| - hotrod.demo.jaegertracing.io | ||
| secretName: jaeger-demo-tls | ||
| secretName: hotrod-ui-only-tls |
There was a problem hiding this comment.
The ingress now has two separate TLS secrets for the two hosts, but there's no validation that these secrets exist. Ensure that both jaeger-ui-only-tls and hotrod-ui-only-tls secrets are created before deploying this ingress, otherwise the TLS configuration will fail.
| secretName: hotrod-ui-only-tls | |
| secretName: hotrod-ui-only-tls | |
| --- | |
| apiVersion: cert-manager.io/v1 | |
| kind: Certificate | |
| metadata: | |
| name: jaeger-ui-only-tls | |
| namespace: jaeger | |
| spec: | |
| secretName: jaeger-ui-only-tls | |
| dnsNames: | |
| - jaeger.demo.jaegertracing.io | |
| issuerRef: | |
| name: letsencrypt-prod | |
| kind: ClusterIssuer | |
| --- | |
| apiVersion: cert-manager.io/v1 | |
| kind: Certificate | |
| metadata: | |
| name: hotrod-ui-only-tls | |
| namespace: jaeger | |
| spec: | |
| secretName: hotrod-ui-only-tls | |
| dnsNames: | |
| - hotrod.demo.jaegertracing.io | |
| issuerRef: | |
| name: letsencrypt-prod | |
| kind: ClusterIssuer |
| ports: | ||
| - containerPort: 8080 | ||
| --- | ||
| apiVersion: v1 |
There was a problem hiding this comment.
There's a trailing space after the service type declaration on line 31 that should be removed for consistency.
| apiVersion: v1 | |
| apiVersion: v1 |
| - name: OTEL_EXPORTER_OTLP_ENDPOINT | ||
| value: "http://jaeger:4318" | ||
| - name: OTEL_EXPORTER_OTLP_TRACES_ENDPOINT | ||
| value: "http://jaeger:4318/v1/traces" |
There was a problem hiding this comment.
The standalone HotROD deployment is missing the OTEL_SERVICE_NAME, OTEL_RESOURCE_ATTRIBUTES, and OTEL_LOG_LEVEL environment variables that are configured in jaeger-values.yaml (lines 32-37). This creates an inconsistency between the HotROD deployment when managed by the Jaeger Helm chart versus the standalone deployment, which could lead to different observability behavior and make it harder to trace issues.
| value: "http://jaeger:4318/v1/traces" | |
| value: "http://jaeger:4318/v1/traces" | |
| - name: OTEL_SERVICE_NAME | |
| value: "jaeger-hotrod" | |
| - name: OTEL_RESOURCE_ATTRIBUTES | |
| value: "service.name=jaeger-hotrod" | |
| - name: OTEL_LOG_LEVEL | |
| value: "info" |
| @@ -1,7 +1,5 @@ | |||
|
|
|||
| image: | |||
There was a problem hiding this comment.
The tag field has been removed from the image configuration, but no explicit version is being set. This means the deployment will use the default tag from the Helm chart values. Given that the deploy-all.sh script now sets --set image.tag="${OPENSEARCH_DASHBOARDS_VERSION}", this is correct. However, it would be clearer to include a comment explaining that the version is managed via the deployment script to avoid confusion.
| image: | |
| image: | |
| # Note: image.tag is intentionally not set here. | |
| # The deploy-all.sh script sets it via: | |
| # --set image.tag="${OPENSEARCH_DASHBOARDS_VERSION}" |
| # Narrower service namespace + explicit environment tag | ||
| - name: OTEL_RESOURCE_ATTRIBUTES | ||
| value: service.name=$(OTEL_SERVICE_NAME),service.namespace=otel-demo,deployment.environment=oke-dev | ||
| value: service.namespace=otel-demo,deployment.environment=oke-dev |
There was a problem hiding this comment.
Removing service.name=$(OTEL_SERVICE_NAME) from the default OTEL_RESOURCE_ATTRIBUTES will cause services to lose their service name in the resource attributes, unless each component explicitly sets OTEL_SERVICE_NAME. This could lead to services being incorrectly identified in tracing. Verify that all components that need service names either have OTEL_SERVICE_NAME set individually or that the SDK defaults are acceptable for your use case.
| value: service.namespace=otel-demo,deployment.environment=oke-dev | |
| value: service.name=$(OTEL_SERVICE_NAME),service.namespace=otel-demo,deployment.environment=oke-dev |
| ROLLOUT_TIMEOUT="${ROLLOUT_TIMEOUT:-600}" | ||
|
|
||
| # Versions | ||
| OPENSEARCH_VERSION="${OPENSEARCH_VERSION:-3.3.2}" |
There was a problem hiding this comment.
| OPENSEARCH_VERSION="${OPENSEARCH_VERSION:-3.3.2}" | |
| DEFAULT_OPENSEARCH_VERSION=3.3.2 | |
| OPENSEARCH_VERSION="${OPENSEARCH_VERSION:-${DEFAULT_OPENSEARCH_VERSION}}" |
| echo " clean - Clean install (removes existing deployment first)" | ||
| echo "" | ||
| echo "Environment Variables:" | ||
| echo " OPENSEARCH_VERSION - Version of OpenSearch (default: 3.3.2)" |
There was a problem hiding this comment.
| echo " OPENSEARCH_VERSION - Version of OpenSearch (default: 3.3.2)" | |
| echo " OPENSEARCH_VERSION - Version of OpenSearch (default: ${DEFAULT_OPENSEARCH_VERSION})" |
| spec: | ||
| containers: | ||
| - name: jaeger-hotrod | ||
| image: docker.io/jaegertracing/example-hotrod:1.72.0 |
There was a problem hiding this comment.
| image: docker.io/jaegertracing/example-hotrod:1.72.0 | |
| image: docker.io/jaegertracing/example-hotrod:2.14.0 |
| name: jaeger-spark-dependencies | ||
| namespace: jaeger | ||
| spec: | ||
| schedule: "*/15 * * * *" |
There was a problem hiding this comment.
how does the job know that you're running it every 15min and that it only needs to look in the last 15min window?
3 participants
PR Description
This pull request introduces fixes, configuration improvements, and new deployments for the OTEL demo environment.
New Deployments
spark-dependencies-cronjob-opensearch.yaml) to calculate dependencies for OpenSearch storage.hotrod.yamldeployment manifest to provide independent lifecycle control over the HotROD application.Configuration Updates
Ingress
jaeger-hotrodservice port to8080.jaeger-ui-only-tlsandhotrod-ui-only-tls) to avoid rate limiting and simplify certificate management.Jaeger
create_mappings: truefor OpenSearch to ensure correct index setup.jaeger-values.yamlto reference thejaegerservice instead ofjaeger-collector.imagePullPolicytoAlways.OpenTelemetry Demo
jaegerservice.OTEL_SERVICE_NAMEfor the frontend component tootelstore-frontend-ui.OTEL_RESOURCE_ATTRIBUTESconfiguration.Upgrades
4.2.33.3.22.11.0to3.3.0for compatibility with backend servicesScript Changes
deploy-all.sh
clone_jaeger_v2logic.jaegerservice.