SEC-1601 #18
SEC-1601 #18
Conversation
WalkthroughIntroduces Prometheus JMX exporter integration to the Solr Docker image. The changes add JMX exporter jar download with checksum verification, include a JMX YAML configuration file, and configure the Java agent via SOLR_OPTS to expose metrics on port 3001. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Alexander-Cairns
force-pushed
the
SEC-1601
branch
from
1c0b495 to
9649fef
Compare
Alexander-Cairns
added
the
minor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile (2)
28-28: Consider setting ownership for JMX files.While the 644 permissions allow the
solruser to read the JAR and configuration file, explicitly setting ownership via--chown=solr:solron the ADD and COPY commands would be more consistent with the pattern used elsewhere in the Dockerfile (e.g., lines 8, 21-22) and clarifies intent.Apply this diff to set explicit ownership:
-ADD \ - --link \ - --chmod=644 \ - --checksum=$JMX_EXPORTER_DIGEST \ - https://github.com/prometheus/jmx_exporter/releases/download/$JMX_EXPORTER_VERSION/jmx_prometheus_javaagent-$JMX_EXPORTER_VERSION.jar jmx_prometheus_javaagent.jar -COPY --chmod=644 jmx.yml ./ +ADD \ + --link \ + --chmod=644 \ + --chown=solr:solr \ + --checksum=$JMX_EXPORTER_DIGEST \ + https://github.com/prometheus/jmx_exporter/releases/download/$JMX_EXPORTER_VERSION/jmx_prometheus_javaagent-$JMX_EXPORTER_VERSION.jar jmx_prometheus_javaagent.jar +COPY --chmod=644 --chown=solr:solr jmx.yml ./This aligns with the ownership pattern used in lines 8, 21-22 of the Dockerfile.
Also applies to: 34-34
36-36: Expose port 3001 for JMX metrics.The JMX agent is configured on port 3001, but the EXPOSE directive (line 6) only declares port 8983. To allow external Prometheus scraping of JMX metrics, add port 3001 to the exposed ports.
Add line 6 (after the existing EXPOSE statement) to expose the JMX metrics port:
EXPOSE 8983 +EXPOSE 3001This ensures container networking is configured to allow external access to the JMX metrics endpoint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Dockerfile(1 hunks)jmx.yml(1 hunks)
🔇 Additional comments (4)
jmx.yml (1)
1-3: Confirm catch-all metric collection pattern.The rule with pattern
".*"exports all available JMX metrics. While this is a reasonable starting point, consider whether you need to restrict or filter specific metrics in the future (especially sensitive internal metrics or high-cardinality metrics that could impact Prometheus storage).Is this catch-all pattern intentional, or should metric filtering be added as a follow-up task?
Dockerfile (3)
25-27: ARG declarations look good.JMX exporter version is pinned to 1.4.0 with explicit SHA256 digest for reproducible builds. The renovate annotation enables automated updates for this dependency.
29-33: JAR download with checksum validation is properly implemented.The ADD command correctly validates the downloaded JMX exporter jar against the provided SHA256 digest, preventing tampering and ensuring build reproducibility. The
--linkflag optimizes layer caching.
25-38: I need to gather more context to verify whether there's an actual conflict risk. Let me search for the container entrypoint and any references to/etc/default/solr.in.sh:Let me search for all occurrences of SOLR_OPTS assignments to ensure there's no override happening:
The review comment's concern about SOLR_OPTS being overridden is unfounded. In Solr it is common to configure settings in solr.in.sh, and the search confirmed that solr.in.sh already uses the correct append pattern:
SOLR_OPTS="$SOLR_OPTS -Dsolr.hocr.plugin.path=...".At run time, before starting Solr, the container will execute scripts in the /docker-entrypoint-initdb.d/ directory, which sources solr.in.sh. When the Dockerfile sets
ENV SOLR_OPTS="-javaagent:...", this value becomes the initial state. When solr.in.sh is sourced, it appends to this value rather than overriding it, so both the javaagent option and any additional options are preserved. The current approach is correct and needs no modification.Likely an incorrect or invalid review comment.
Summary by CodeRabbit