khandelwal-prateek commented on code in PR #4061:
URL: https://github.com/apache/gobblin/pull/4061#discussion_r1778709820
##########
gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java:
##########
@@ -911,6 +911,12 @@ public class ConfigurationKeys {
public static final String METRICS_REPORTING_OPENTELEMETRY_PREFIX =
"metrics.reporting.opentelemetry.";
public static final String METRICS_REPORTING_OPENTELEMETRY_ENABLED =
METRICS_REPORTING_OPENTELEMETRY_PREFIX + "enabled";
+ public static final String
METRICS_REPORTING_OPENTELEMETRY_LOGEXPORTER_ENABLED =
METRICS_REPORTING_OPENTELEMETRY_PREFIX + "logexporter.enabled";
+
+ public static final Boolean
DEFAULT_METRICS_REPORTING_OPENTELEMETRY_LOGEXPORTER_ENABLED = false;
+
+ public static final String
METRICS_REPORTING_OPENTELEMETRY_LOGEXPORTER_CLASSNAME =
METRICS_REPORTING_OPENTELEMETRY_PREFIX + "logexporter.className";
Review Comment:
I understand that it would be useful to make the MetricExporter
instantiation more generic, allowing for any MetricExporter class to be
instantiated. However, since different exporters may require varying
instantiation methods (e.g., constructors vs. static methods), and we also need
to set certain properties which are specific to `OtlpHttpMetricExporter`, so
it's challenging to handle them all in a truly generic way.
One possible solution could be to use a factory pattern, where we have a
separate factory class for each MetricExporter type. Each factory would know
how to properly instantiate its corresponding MetricExporter. However, this
would require significant changes
```
public interface MetricExporterFactory {
MetricExporter create(State state);
}
@Override
protected MetricExporter initializeMetricExporter(State state) {
MetricExporterFactory factory = getMetricExporterFactory(state);
return factory.create(state);
}
```
Or we can update `initializeMetricExporter` method to use reflection to
instantiate the class specified in the configuration, however, since the
instantiation is not common across different metricExporters, supporting
another metricExporter might still require new code to be handled.
```
protected MetricExporter initializeMetricExporter(State state) {
MetricExporter metricExporter;
String exporterClassName =
state.getProp(ConfigurationKeys.METRICS_REPORTING_OPENTELEMETRY_EXPORTER_CLASSNAME,
OtlpHttpMetricExporter.class.getName());
try {
Class<?> clazz = Class.forName(exporterClassName);
Method instanceMethod = clazz.getMethod("instance");
// Invoke the method to get the singleton instance
metricExporter = (MetricExporter) instanceMethod.invoke(null);
} catch (Exception e) {
log.error("Error occurred while instantiating opentelemetry Exporter
class: " + exporterClassName, e);
metricExporter = createDefaultExporter(state);
}
return metricExporter;
}
```
Let me know if the existing way to create LogMetricExporter is fine given
the constraints or you meant something else.
--
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]
