feat: Add system property override for service.instance.id#1227
feat: Add system property override for service.instance.id#1227kuisathaverat wants to merge 4 commits intojenkinsci:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to override service.instance.id via a system property (io.jenkins.plugins.opentelemetry.service.instance.id) to support CloudBees CI high availability scenarios where multiple controller replicas require unique instance identifiers while maintaining the same service name.
Changes:
- Added system property check with fallback to
Jenkins.getLegacyInstanceId()for backward compatibility - Created integration tests to validate default behavior, system property override, and edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| src/main/java/io/jenkins/plugins/opentelemetry/JenkinsOpenTelemetryPluginConfiguration.java | Adds system property check for service.instance.id with proper validation and fallback logic |
| src/test/java/io/jenkins/plugins/opentelemetry/ServiceInstanceIdConfigurationTest.java | New test file with integration tests (contains multiple critical issues preventing compilation) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * | ||
| * @see <a href="https://github.com/jenkinsci/opentelemetry-plugin/issues/1154">Issue #1154</a> | ||
| */ | ||
| @WithJenkins |
There was a problem hiding this comment.
The test uses @WithJenkins annotation from JUnit Jupiter (org.jvnet.hudson.test.junit.jupiter.WithJenkins), but this is inconsistent with the rest of the test suite which uses JUnit 4 with @rule or @ClassRule JenkinsRule patterns. Looking at BaseIntegrationTest.java:77-79 and OtelLocaLogMirroringTest.java:36-37, the established pattern is to use JenkinsConfiguredWithCodeRule or JenkinsRule with JUnit 4 annotations. This test should follow the same pattern to ensure it integrates properly with the existing test infrastructure.
| private String getServiceInstanceIdFromLastExportedMetric() { | ||
| List<InMemoryMetricExporter.FinishedMetricItem> metrics = inMemoryMetricExporter.getFinishedMetricItems(); | ||
| assertThat("At least one metric should be exported", metrics, notNullValue()); | ||
| assertThat("Metrics list should not be empty", !metrics.isEmpty()); |
There was a problem hiding this comment.
The assertion assertThat("Metrics list should not be empty", !metrics.isEmpty()) is syntactically incorrect for Hamcrest. The correct form should be assertThat("Metrics list should not be empty", metrics, is(not(empty()))) or simply check the condition with a standard assertion. The current form passes a boolean as the actual value instead of using a proper Hamcrest matcher.
Allow service.instance.id to be overridden via system property io.jenkins.plugins.opentelemetry.service.instance.id for environments like CloudBees CI HA where multiple replicas need unique IDs. - UPDATE: JenkinsOpenTelemetryPluginConfiguration.java - Add system property check before using Jenkins.getLegacyInstanceId() - Fall back to legacy ID when property is null or empty - Maintain 100% backward compatibility - NEW: ServiceInstanceIdConfigurationTest.java - Test default behavior (backward compatibility) - Test system property override - Test edge cases (empty string, whitespace-only values) - 4 comprehensive integration tests using Jenkins Test Harness Closes jenkinsci#1154
eaa5176 to
fb03bac
Compare
|
Great addition @kuisathaverat! The system property override approach for A few observations after reviewing the changes:
|
Summary
Add ability to override
service.instance.idvia system property for CloudBees CI high availability scenarios where multiple controller replicas need unique instance identifiers.Changes
UPDATE:
JenkinsOpenTelemetryPluginConfiguration.javaio.jenkins.plugins.opentelemetry.service.instance.idJenkins.getLegacyInstanceId()when property not setNEW:
ServiceInstanceIdConfigurationTest.javaTesting
Additional Information
io.jenkins.plugins.opentelemetry.service.instance.id-Dio.jenkins.plugins.opentelemetry.service.instance.id=replica-01)Closes #1154