-
Notifications
You must be signed in to change notification settings - Fork 563
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
Auto instrumentation parameters #3864
base: main
Are you sure you want to change the base?
Conversation
opentelemetry-sdk/src/opentelemetry/sdk/_configuration/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/_configuration/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/_configuration/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/_configuration/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/_configuration/__init__.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, posted one idea. Thanks for presenting this at multiple sig meetings.
_get_exporter_names("logs"), | ||
def _initialize_components( | ||
auto_instrumentation_version: Optional[str] = None, | ||
span_exporter_names: Optional[List[str]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we stick with trace_exporter_names
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine either way. But the object seems to be called SpanExporter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ocelotl Any thoughts? We use both "span_exporter" and "trace_exporter" naming conventions in internal variables, but the object is called SpanExporter. I am open to either but want to make sure to get broad input on this since it is an api change that we cannot change without breaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call them "trace_exporter" because user is configuring what exporter they want under the "trace" flag. As well, the entry points are using opentelemetry_traces_exporter
.
36fab8c
to
c6980b5
Compare
}, | ||
) | ||
@patch.dict( | ||
environ, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, | |
) | |
@patch.dict( | |
environ, | |
{ |
What about patching the environ dict once?
Description
Currently, the only way for custom Configurators to configure is through setting or defaulting environment variables. While it makes sense for customers to configure autoinstrumentation with env vars, there is no reason Configurators should not be able to directly configure start up (exporters, samplers...etc). Exposing autoinstrumentation configuration of otel via env var defaults is invasive, confusing, and easy for users to mess up. For instance, even a blank env var or a left-over setting from previous run could interfere with a distro starting up.
In this approach, configurators can pass kwargs to _initialize_components to easily configure autoinstrumentation which currently only considers env vars.
Precedence of params over env vars
Params would be preferred over (or merged with) env vars. This follows the approach towards env vars elsewhere in OTel Python. And doing so would be necessary for custom distros to guarantee certain behavior.
In this approach, exporters given by params would be ADDED to those from env vars. Resource attributes would be merged with those from env vars and resource detectors. Since there's only 1 sampler, param would take precedence. Same for logging_enabled.