Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a simple way to configure system-metrics from outside #2528

Closed
xrmx opened this issue May 16, 2024 · 4 comments
Closed

Add a simple way to configure system-metrics from outside #2528

xrmx opened this issue May 16, 2024 · 4 comments

Comments

@xrmx
Copy link
Contributor

xrmx commented May 16, 2024

Is your feature request related to a problem?

I would like to easily provide a different config for system metrics, i.e. to provide a subset of metrics, depending on an environment variable.

The instrumentation config can be passed to the instrumentation constructor.

Here's the config:
https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py#L99

In today SIG @aabmass suggested to take a look at BaseDistro.load_instrumentor() that may already do the trick

Describe the solution you'd like

No solution in mind

Describe alternatives you've considered

None

Additional context

@xrmx
Copy link
Contributor Author

xrmx commented May 16, 2024

cc @jeremydvoss

xrmx added a commit to xrmx/opentelemetry-python-contrib that referenced this issue May 20, 2024
This is called for every entrypoint found by auto instrumentation and
so it takes just one entry point.

Refs open-telemetry#2528
@xrmx
Copy link
Contributor Author

xrmx commented May 20, 2024

So using load_instrumentor will look like this:

diff --git a/src/elasticotel/distro/__init__.py b/src/elasticotel/distro/__init__.py
index ef1bd6a..60816fa 100644
--- a/src/elasticotel/distro/__init__.py
+++ b/src/elasticotel/distro/__init__.py
@@ -19,11 +19,14 @@ from opentelemetry.environment_variables import (
     OTEL_TRACES_EXPORTER,
 )
 from opentelemetry.instrumentation.distro import BaseDistro
+from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
+from opentelemetry.instrumentation.system_metrics import _DEFAULT_CONFIG, SystemMetricsInstrumentor
 from opentelemetry.sdk._configuration import _OTelSDKConfigurator
 from opentelemetry.sdk.environment_variables import (
     OTEL_EXPERIMENTAL_RESOURCE_DETECTORS,
     OTEL_EXPORTER_OTLP_PROTOCOL,
 )
+from pkg_resources import EntryPoint
 
 
 class ElasticOpenTelemetryConfigurator(_OTelSDKConfigurator):
@@ -31,6 +34,14 @@ class ElasticOpenTelemetryConfigurator(_OTelSDKConfigurator):
 
 
 class ElasticOpenTelemetryDistro(BaseDistro):
+    def load_instrumentor(self, entry_point: EntryPoint, **kwargs):
+
+        instrumentor_class: BaseInstrumentor = entry_point.load()
+        instrumentor_kwargs = {}
+        if instrumentor_class == SystemMetricsInstrumentor:
+            instrumentor_kwargs["config"] = {k: v for k, v in _DEFAULT_CONFIG.items() if k.startswith("process.runtime")}
+        instrumentor_class(**instrumentor_kwargs).instrument(**kwargs)
+
     def _configure(self, **kwargs):
         os.environ.setdefault(OTEL_TRACES_EXPORTER, "otlp")
         os.environ.setdefault(OTEL_METRICS_EXPORTER, "otlp")

An improvement would be to being able to reduce a bit of duplicated code would be something like this where on the distro side I would only override the helper returning the kwargs for the instrumentor constructor:

diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/distro.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/distro.py
index 93646bbb..02fa3b8d 100644
--- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/distro.py
+++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/distro.py
@@ -47,6 +47,11 @@ class BaseDistro(ABC):
         """Configure the distribution"""
         self._configure(**kwargs)
 
+    def load_instrumentor_kwargs(self, instrumentor: BaseInstrumentor) -> dict:
+        """Takes an instrumentor class and returns a dict of kwargs to pass to
+        the constructor"""
+        return {}
+
     def load_instrumentor(  # pylint: disable=no-self-use
         self, entry_point: EntryPoint, **kwargs
     ):
@@ -61,6 +66,7 @@ class BaseDistro(ABC):
         skip loading entirely, etc.
         """
         instrumentor: BaseInstrumentor = entry_point.load()
-        instrumentor().instrument(**kwargs)
+       instrumentor_kwargs = self.load_instrumentor_kwargs(instrumentor)
+       instrumentor(**instrumentor_kwargs).instrument(**kwargs)
 

@jeremydvoss
Copy link
Contributor

load_instrumentor_kwargs seems to me like a method that can be entirely handled by the custom ElasticOpenTelemetryDistro. I think we should only change the sdk/api if it were preventing instrumentation kwarg configuration

@xrmx
Copy link
Contributor Author

xrmx commented May 24, 2024

Would making it an internal helper ok? It would be useful as documentation at least.

@xrmx xrmx closed this as completed May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants