[GOBBLIN-2161] Support for configuring openTelemetry metric exporter as logExporter

[khandelwal-prateek]

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

• My PR addresses the following Gobblin JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
  • https://issues.apache.org/jira/browse/GOBBLIN-2161

Description

• Here are some details about my PR, including screenshots (if applicable):
  • Added support for configuring openTelemetry metric exporter as logExporter, which would be useful to log metric data for debugging. The logging metricExporter is configured using a flag which is set to false by default and OtlpHttpMetricExporter would be the default metricExporter class.

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

• My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

[Will-Lo]

Instead of making a direct reference around a log exporter, how about we make the metric exporter a generic className such that any metric exporter class can be instantiated?

[Will-Lo]

The default class if no class is defined can be the OtlpHttpMetricExporter

[khandelwal-prateek]

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.

[Will-Lo]

Hmm I see, for the context of this PR a logging exporter shouldn't require a different constructor like the second method you mentioned but the long term soln is definitely to use the factory pattern. Please add a TODO for that. Otherwise seems reasonable to me.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 55.28%. Comparing base (45ad13e) to head (ca373ea).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
...pache/gobblin/configuration/ConfigurationKeys.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #4061       +/-   ##
=============================================
+ Coverage     45.12%   55.28%   +10.16%     
+ Complexity     3199     1582     -1617     
=============================================
  Files           705      307      -398     
  Lines         26949    10581    -16368     
  Branches       2680     1069     -1611     
=============================================
- Hits          12160     5850     -6310     
+ Misses        13781     4226     -9555     
+ Partials       1008      505      -503     

Flag	Coverage Δ
	55.28% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.