feat: add jaeger-all-in-one for trouble shooting #209
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
scripts/release/release-charts-to-acr.sh (1)
13-17: Consider refactoring URL replacements for better maintainability.The script has multiple hardcoded URLs that could be made more maintainable.
Consider refactoring like this:
if [ "$chart_name" == "greptimedb-cluster" ]; then + # Define URL mappings + declare -A url_mappings=( + ["https://grafana.github.io/helm-charts"]="oci://$OCI_REGISTRY_URL/$OCI_NAMESPACE" + ["https://raw.githubusercontent.com/hansehe/jaeger-all-in-one/master/helm/charts"]="oci://$OCI_REGISTRY_URL/$OCI_NAMESPACE" + ) + + # Replace URLs + for old_url in "${!url_mappings[@]}"; do + new_url="${url_mappings[$old_url]}" + sed -i "s|$old_url|$new_url|g" "$dir/Chart.yaml" + done - sed -i 's|https://grafana.github.io/helm-charts|oci://greptime-registry.cn-hangzhou.cr.aliyuncs.com/charts|g' "$dir/Chart.yaml" - sed -i 's|https://raw.githubusercontent.com/hansehe/jaeger-all-in-one/master/helm/charts|oci://greptime-registry.cn-hangzhou.cr.aliyuncs.com/charts|g' "$dir/Chart.yaml" # Update the dependencies. helm dependency update "$dir" ficharts/greptimedb-cluster/README.md (1)
77-77: Format the URL using proper markdown syntaxConvert the bare URL to proper markdown link format for better documentation standards.
-| https://raw.githubusercontent.com/hansehe/jaeger-all-in-one/master/helm/charts | jaeger-all-in-one | 0.1.12 | +| [jaeger-all-in-one](https://raw.githubusercontent.com/hansehe/jaeger-all-in-one/master/helm/charts) | jaeger-all-in-one | 0.1.12 |🧰 Tools
🪛 Markdownlint (0.35.0)
77-77: null
Bare URL used(MD034, no-bare-urls)
charts/greptimedb-cluster/values.yaml (1)
1043-1049: Consider adding useful annotations for the Jaeger serviceAdding prometheus.io annotations would help in monitoring the Jaeger instance.
service: type: ClusterIP port: 16686 - annotations: {} + annotations: + prometheus.io/scrape: "true" + prometheus.io/port: "16686"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
charts/greptimedb-cluster/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
charts/greptimedb-cluster/Chart.yaml(2 hunks)charts/greptimedb-cluster/README.md(3 hunks)charts/greptimedb-cluster/values.yaml(1 hunks)scripts/release/release-charts-to-acr.sh(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
charts/greptimedb-cluster/README.md
77-77: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (4)
charts/greptimedb-cluster/Chart.yaml (1)
5-5: LGTM on version bump
The version increment from 0.2.36 to 0.2.37 follows semantic versioning and is appropriate for adding a new optional dependency.
charts/greptimedb-cluster/README.md (1)
5-5: LGTM: Version increment is appropriate
Version bump from 0.2.36 to 0.2.37 aligns with the feature addition of jaeger-all-in-one support.
charts/greptimedb-cluster/values.yaml (2)
1051-1058: LGTM! Volume configuration looks good
The persistence configuration is well-defined with a reasonable default size of 3Gi for development purposes.
1060-1064: LGTM! Collector configuration is appropriate
Enabling both Zipkin and OpenTelemetry collectors provides good flexibility for different tracing implementations.
zyy17
force-pushed
the
feat/add-jaeger-dev
branch
from
626f29f to
ef4d7fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
scripts/e2e/greptimedb-cluster.sh (1)
56-58: Consider adding Jaeger deployment verification.Since Jaeger is being added for troubleshooting purposes, it would be valuable to:
- Verify that Jaeger pods are running correctly after deployment
- Add basic smoke tests to ensure Jaeger is collecting traces
Here's a suggested addition to the
deploy_greptimedb_clusterfunction:helm dependency build charts/greptimedb-cluster # Deploy greptimedb cluster. helm upgrade --install mycluster charts/greptimedb-cluster -n default + +# Verify Jaeger deployment if enabled +if [ "$(helm get values mycluster -n default -a -o json | jq -r '.jaeger-all-in-one.enabled')" = "true" ]; then + echo "Waiting for Jaeger to be ready..." + kubectl rollout status --timeout=60s deployment/mycluster-jaeger -n default +ficharts/greptimedb-cluster/README.md (1)
77-77: Format repository URL properlyThe bare URL should be properly formatted in markdown style.
-| https://raw.githubusercontent.com/hansehe/jaeger-all-in-one/master/helm/charts | jaeger-all-in-one | 0.1.12 | +| [jaeger-all-in-one](https://raw.githubusercontent.com/hansehe/jaeger-all-in-one/master/helm/charts) | jaeger-all-in-one | 0.1.12 |🧰 Tools
🪛 Markdownlint (0.35.0)
77-77: null
Bare URL used(MD034, no-bare-urls)
charts/greptimedb-cluster/values.yaml (2)
1051-1058: Add documentation about volume usageConsider adding a comment explaining what data is being persisted and any retention policies.
# -- The jaeger-all-in-one persistence configuration. volume: - # -- Whether to enable the persistence for jaeger-all-in-one. + # -- Whether to enable the persistence for jaeger-all-in-one. + # This volume stores trace data and should be sized based on your retention needs. enabled: true
1060-1064: Enhance collector configuration commentsAdd port numbers to the comments for better clarity.
- # -- Enable the zipkin collector for jaeger-all-in-one and listen on port 9411. + # -- Enable the Zipkin collector for jaeger-all-in-one (listens on port 9411 for HTTP). enableHttpZipkinCollector: true - # -- Enable the opentelemetry collector for jaeger-all-in-one and listen on port 4317. + # -- Enable the OpenTelemetry collector for jaeger-all-in-one (listens on port 4317 for gRPC). enableHttpOpenTelemetryCollector: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
charts/greptimedb-cluster/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/ci.yaml(1 hunks)charts/greptimedb-cluster/Chart.yaml(2 hunks)charts/greptimedb-cluster/README.md(3 hunks)charts/greptimedb-cluster/values.yaml(1 hunks)scripts/e2e/greptimedb-cluster.sh(1 hunks)scripts/release/release-charts-to-acr.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/greptimedb-cluster/Chart.yaml
- scripts/release/release-charts-to-acr.sh
🧰 Additional context used
🪛 Markdownlint (0.35.0)
charts/greptimedb-cluster/README.md
77-77: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (7)
.github/workflows/ci.yaml (1)
31-31:
Security & Reliability Concerns with Helm Repository Source
The current implementation raises several concerns:
- The repository URL points to a personal GitHub account which poses security and reliability risks
- Using raw.githubusercontent.com for Helm repositories is not recommended for production use
- There's no integrity verification of the charts
Consider these alternatives:
- Use the official Jaeger Helm repository or OCI registry
- If using a third-party source is necessary:
- Mirror the charts to your organization's registry
- Add integrity verification
- Document the security implications
Example implementation using the official Jaeger repository:
-helm repo add jaeger-all-in-one https://raw.githubusercontent.com/hansehe/jaeger-all-in-one/master/helm/charts/
+helm repo add jaegertracing https://jaegertracing.github.io/helm-chartsLet's verify if the official Jaeger chart provides the required functionality:
charts/greptimedb-cluster/README.md (2)
233-236: Previous review comments are still applicable
The concerns about using 'latest' tag for container images raised in the previous review are still valid.
237-237: Previous review comments are still applicable
The concerns about missing resource limits for jaeger-all-in-one raised in the previous review are still valid.
charts/greptimedb-cluster/values.yaml (4)
1028-1031: LGTM!
The basic configuration follows Helm chart best practices with clear comments and structure.
1034-1040: Avoid using 'latest' tag for container images
Using the 'latest' tag for container images can lead to unpredictable behavior and make it difficult to track which version is actually deployed.
1043-1049: LGTM!
The service configuration uses appropriate defaults and the standard Jaeger UI port.
1066-1073: Define resource limits for the Jaeger container
Missing resource limits could lead to resource contention or container eviction in a Kubernetes cluster.
zyy17
force-pushed
the
feat/add-jaeger-dev
branch
from
ef4d7fe to
7f5b269
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
charts/greptimedb-cluster/README.md (1)
77-77: Fix markdown lint issue: Use proper link formatThe URL should be properly formatted as a markdown link.
-| https://raw.githubusercontent.com/hansehe/jaeger-all-in-one/master/helm/charts | jaeger-all-in-one | 0.1.12 | +| [jaeger-all-in-one charts](https://raw.githubusercontent.com/hansehe/jaeger-all-in-one/master/helm/charts) | jaeger-all-in-one | 0.1.12 |🧰 Tools
🪛 Markdownlint (0.35.0)
77-77: null
Bare URL used(MD034, no-bare-urls)
charts/greptimedb-cluster/values.yaml (1)
1043-1049: Consider adding security context for the serviceThe service configuration should include security-related annotations, especially since Jaeger handles trace data which might contain sensitive information.
service: type: ClusterIP port: 16686 - annotations: {} + annotations: + security.alpha.kubernetes.io/pod-security-standards: restricted + security.alpha.kubernetes.io/network-policy: enabled
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
charts/greptimedb-cluster/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/ci.yaml(1 hunks)charts/greptimedb-cluster/Chart.yaml(2 hunks)charts/greptimedb-cluster/README.md(3 hunks)charts/greptimedb-cluster/values.yaml(1 hunks)scripts/e2e/greptimedb-cluster.sh(1 hunks)scripts/release/release-charts-to-acr.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/ci.yaml
- charts/greptimedb-cluster/Chart.yaml
- scripts/e2e/greptimedb-cluster.sh
- scripts/release/release-charts-to-acr.sh
🧰 Additional context used
🪛 Markdownlint (0.35.0)
charts/greptimedb-cluster/README.md
77-77: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (5)
charts/greptimedb-cluster/README.md (2)
5-5: LGTM: Version bump is correctly documented
The version badge has been updated to reflect the new version 0.2.37.
229-245:
Security and operational concerns in jaeger-all-in-one configuration
Several issues need to be addressed:
- The image tag is set to 'latest' which is not recommended for production use
- No resource limits are defined which could lead to resource exhaustion
- The volume size of 3Gi might be insufficient for production workloads
Additional recommendations:
- Consider adding network policies to restrict access to the collectors
- Document the security implications of enabling both OpenTelemetry and Zipkin collectors
Would you like me to provide a complete example of network policies for securing the Jaeger deployment?
charts/greptimedb-cluster/values.yaml (3)
1028-1031: LGTM: Basic deployment configuration looks good
The configuration follows Helm chart best practices with proper comments and default values.
1034-1040: Avoid using 'latest' tag for container images
Using the 'latest' tag can lead to unpredictable behavior and make it difficult to track which version is actually deployed.
1066-1073: Define resource limits for the Jaeger container
Missing resource limits could lead to resource contention or container eviction in a Kubernetes cluster.
Summary by CodeRabbit
New Features
greptimedb-clusterHelm chart to version0.2.37.jaeger-all-in-onefor enhanced tracing capabilities.jaeger-all-in-one, including deployment enablement and image settings.Documentation
jaeger-all-in-oneconfiguration options.Chores
Chart.yaml.