Blazer-007 commented on code in PR #4118:
URL: https://github.com/apache/gobblin/pull/4118#discussion_r2625406034
##########
gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/workflow/impl/ProcessWorkUnitsWorkflowImpl.java:
##########
@@ -50,15 +54,22 @@
import org.apache.gobblin.temporal.workflows.metrics.TemporalEventTimer;
import org.apache.gobblin.util.PropertiesUtils;
+import static
org.apache.gobblin.metrics.opentelemetry.GobblinOpenTelemetryMetricsConstants.DimensionKeys.*;
+import static
org.apache.gobblin.metrics.opentelemetry.GobblinOpenTelemetryMetricsConstants.DimensionValues.*;
+
@Slf4j
public class ProcessWorkUnitsWorkflowImpl implements ProcessWorkUnitsWorkflow {
public static final String CHILD_WORKFLOW_ID_BASE = "NestingExecWorkUnits";
public static final String COMMIT_STEP_WORKFLOW_ID_BASE =
"CommitStepWorkflow";
+ private EmitOTelMetrics emitOTelMetricsActivityStub;
Review Comment:
The current implementation is tight coupling wrt name only , we can look at
the name change end to end later on if required as OpenTelemetry name is being
used across orchestrator as well to emit final status metric.
In suggested interface & its impl we are just adding extra hop and the
object of that impl can't be directly used inside temporal workflow as
OpenTelemetryJobMetricsRecorder -> OpenTelemetryInstrumentation -> meter and
meter object is not serializable so again we will need a temporal activity stub
to connect to this JobMetricRecorder and that is what current impl is doing as
well (if we ignore the name of class) , if we want to use other metric system
in future we can just change EmitOTelMetricsImpl itself regardless of name.
I will raise a followup PR to address the name change of Activity interface
and impl
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
