config to API, .env support #1079
Conversation
brettmc
commented
Jul 13, 2023
brettmc
commented
- We have a need in some contrib packages to fetch configuration settings. Move the non-SDK-specific parts of Configuration into the API.
- add the ability to fetch config from .env file
* We have a need in some contrib packages to fetch configuration settings. Move the non-SDK-specific parts of Configuration into the API. * add the ability to fetch config from .env file
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1079 +/- ##
============================================
+ Coverage 81.67% 81.73% +0.05%
- Complexity 2187 2208 +21
============================================
Files 278 282 +4
Lines 6231 6284 +53
============================================
+ Hits 5089 5136 +47
- Misses 1142 1148 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| /** | ||
| * @psalm-internal \OpenTelemetry | ||
| */ | ||
| class ClassConstantAccessor |
There was a problem hiding this comment.
Only used in SDK Configuration -> can stay in SDK?
| * Resolve variables from a `.env` file | ||
| * @psalm-internal \OpenTelemetry\API\Configuration | ||
| */ | ||
| class DotEnvResolver implements ResolverInterface |
There was a problem hiding this comment.
I don't think we should add this resolver, users that use .env files are likely already using vlucas/phpdotenv or symfony/dotenv.
There was a problem hiding this comment.
The use-case I can think of is if you're using the SDK autoloading feature, including for auto-instrumentation. Since that all happens during composer autoloading, dotenv libraries won't have run yet.
That said, I don't have a strong opinion on having .env support or not, it just seemed like a quick win and potentially useful.
There was a problem hiding this comment.
The dotenv libraries seem to be more complex and handle more cases, I'm mainly concerned that the DotEnvResolver might only support a subset of the .env specification.
Do we need access to the configuration only within hooks or also before initializing hooks? |
Right now, only within hooks. But I can imagine a need to have this configuration when initializing hooks: to be able to control which methods are observed. It might not make a big difference in reality, but I think that not observing a method would be more efficient than observing it but making it a no-op based on a check. The problem I've got is this part of the spec: Or, if it worked in the other direction, and there was a There's also scope to have SIG-specific exclusions of parts of the spec, so we could decide to ignore that part of the spec. edit: created draft PR #1083 which is simpler and avoids moving a heap of code into the API |
|
Closing in favour of #1083 |
2 participants
