-
Notifications
You must be signed in to change notification settings - Fork 651
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
Load environment variables as options for opentelemetry-instrument #1969
Conversation
f5f888f
to
4c1463f
Compare
I feel like we might be abusing the plugin (entry point) system here a bit too much. Do we know what are performance hits this could result in? It is constant or linear with the size of your site-packages directory? For environments like Lamba, these things might add up and incur a significant startup penalty. Other than this, I also have concerns about documentation, UX and stability guarantees. How and where do we document env variables if this package is dynamically "sourced" from other packages that may or may not be installed? If an env variable is defined in let's say
I think it would be very surprising if as a user, I import something from It generally feels a bit too magical and I personally don't think it is worth just to have a common import path. BTW do we really not know all packages that we want to source environment variables from? It feels like we should know that and should be able to import with a try..except if we really wanted to dynamically generate this package. |
b5661f4
to
0dead19
Compare
It is in theory possible to include each environment variable documentation as the command option help by using the documentation defined for every declared environment variable. For example, the `OTEL_RESOURCE_ATTRIBUTES has the following documentation:
That documentation could be added to the command option help. Of course the formatting won't be ideal, but maybe a solution can be found. Just pointing this out to give you all the complete picture. |
Should we lower case the vars as a convention? All caps args feel a bit weird. |
But all caps matches the environment variables exactly 😎 |
Sure, but is that such a big deal? If we were to do this manually to create CLI arg counter-parts for all env vars, we wouldn't use all caps, right? IMO the fact that we source these args from the environment variables module is just a technical detail. Env vars and CLI args both could be sourced from another "config" module or a YAML file. In that case we wouldn't use all caps I think. So IMO we shouldn't let the fact that the code fetches these from env vars affect the casing of cli args. I feel like each config mechanism following established patterns would be better. That said, not a huge deal for me. |
Actually, now that I think more about it, do we even need the |
I find the lower case flags easier to process as a user, regardless of the fact that the uppercase variables match the environment variables. I also agree with @owais that removing as much duplication as possible in the name of the arguments makes sense if we want this to be user friendly. It would be easy enough for the help to show what env variable can be used to override any of the flags if we're concerned about the discrepancy there. |
Added the requested changes, now the help looks like this:
|
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.
Looks great. Left a minor documentation comment.
...telemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/__init__.py
Show resolved
Hide resolved
Since the change in `opentelemetry-instrument` to understand environment variables [0], the options documented in this README file are outdated. [0] open-telemetry/opentelemetry-python#1969
Fixes #1968
This PR makes all
OTEL_
-prefixed environment variables to be loaded as options foropentelemetry-instrument
.The
opentelemetry-instrument --help
command now shows this: