job execute collector #2265
job execute collector #2265
Conversation
| <dependency> | ||
| <groupId>org.apache.shardingsphere.elasticjob</groupId> | ||
| <artifactId>elasticjob-tracing-observability</artifactId> | ||
| <version>3.1.0-SNAPSHOT</version> |
There was a problem hiding this comment.
- I recommend using
${project.version}.
linghengqian
left a comment
linghengqian
left a comment
There was a problem hiding this comment.
- Can you merge the maater branch to fix CI?
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new metrics-based tracing system to ElasticJob, complementing the existing RDB tracing. It introduces a new observability module that collects job metrics and exposes them via Prometheus endpoints.
- Adds new
elasticjob-tracing-observabilitymodule with metrics collection capabilities - Introduces constants for tracing types to improve code maintainability
- Adds job initialization tracking with new TASK_INIT state
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ElasticJobTracingConfiguration.java | Adds metrics tracing configuration support alongside existing RDB tracing |
| pom.xml (spring-boot-starter) | Adds dependency on new observability module |
| JobScheduler.java | Adds job executor initialization call |
| pom.xml (tracing) | Includes new observability module in build |
| RDBTracingListenerConfiguration.java | Updates to use TracingType constants |
| Multiple observability files | New module implementing metrics collection with Prometheus integration |
| TracingType.java | New constants class for tracing type strings |
| JobStatusTraceEvent.java | Adds TASK_INIT state for job initialization tracking |
| ElasticJobExecutor.java | Adds initialization method with status tracing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @Bean | ||
| @ConditionalOnBean(DataSource.class) | ||
| @ConditionalOnProperty(name = "elasticjob.tracing.type", havingValue = "RDB") | ||
| @ConditionalOnProperty(name = "elasticjob.tracing.type", havingValue = RDB) |
There was a problem hiding this comment.
The havingValue attribute expects a String literal, but RDB is being used as a constant reference. This should be havingValue = \"RDB\" or use the string value from TracingType.RDB.
| */ | ||
| @Bean | ||
| @ConditionalOnBean(MetricConfig.class) | ||
| @ConditionalOnProperty(name = "elasticjob.tracing.type", havingValue = TracingType.METRICS) |
There was a problem hiding this comment.
The havingValue attribute expects a String literal, but TracingType.METRICS is being used as a constant reference. This should be havingValue = \"METRICS\" or use the string value directly.
| @ConditionalOnProperty(name = "elasticjob.tracing.type", havingValue = TracingType.METRICS) | |
| @ConditionalOnProperty(name = "elasticjob.tracing.type", havingValue = "METRICS") |
|
|
||
|
|
||
| /** | ||
| * RDB tracing listener configuration. |
There was a problem hiding this comment.
The class comment incorrectly refers to 'RDB tracing listener configuration' when this is actually a metrics tracing listener configuration.
| * RDB tracing listener configuration. | |
| * Metrics tracing listener configuration. |
| jobMetrics = new JobMetrics(); | ||
|
|
||
|
|
||
| //开启一个服务，用于暴露指标 |
There was a problem hiding this comment.
Chinese comments should be replaced with English comments for consistency with the rest of the codebase. The comment should be: '// Start a service to expose metrics'.
| //开启一个服务，用于暴露指标 | |
| // Start a service to expose metrics |
| os.write(response.getBytes()); | ||
| } | ||
| }); | ||
| //开启服务 |
There was a problem hiding this comment.
Chinese comment should be replaced with English: '// Start the service'.
| //开启服务 | |
| // Start the service |
| PrometheusMeterRegistry prometheusRegistry = new PrometheusMeterRegistry(PrometheusConfig.DEFAULT); | ||
| composite.add(prometheusRegistry); | ||
|
|
||
| server = HttpServer.create(new InetSocketAddress(8081), 0); |
There was a problem hiding this comment.
The port 8081 is hardcoded, but the MetricConfig class has a metricsPort field. This should use metricConfig.getMetricsPort() instead of the hardcoded value.
| server = HttpServer.create(new InetSocketAddress(8081), 0); | |
| server = HttpServer.create(new InetSocketAddress(metricConfig.getMetricsPort()), 0); |
|
|
||
| server = HttpServer.create(new InetSocketAddress(8081), 0); | ||
|
|
||
| server.createContext("/metrics", httpExchange -> { |
There was a problem hiding this comment.
The metrics path '/metrics' is hardcoded, but the MetricConfig class has a metricsPath field. This should use metricConfig.getMetricsPath() instead of the hardcoded value.
| server.createContext("/metrics", httpExchange -> { | |
| server.createContext(metricConfig.getMetricsPath(), httpExchange -> { |
| } | ||
|
|
||
| /** | ||
| * Rigourous Test :-) |
There was a problem hiding this comment.
Spelling error: 'Rigourous' should be 'Rigorous'.
| * Rigourous Test :-) | |
| * Rigorous Test :-) |
| <dependency> | ||
| <groupId>junit</groupId> | ||
| <artifactId>junit</artifactId> | ||
| <version>3.8.1</version> |
There was a problem hiding this comment.
JUnit version 3.8.1 is extremely outdated. Consider updating to a modern version like JUnit 5 or at least JUnit 4.13.2 for better testing capabilities and security updates.
| <version>3.8.1</version> | |
| <version>4.13.2</version> |
2 participants
Fixes #ISSUSE_ID.
fix #2237
Changes proposed in this pull request: