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

refactor: static access for EnvironmentVariablesTrait methods #850

Merged
merged 3 commits into from
Oct 26, 2022

Conversation

tcarrio
Copy link
Contributor

@tcarrio tcarrio commented Oct 25, 2022

The EnvironmentVariablesTrait provided instance accessor methods with only static calls. This refactors those methods to be static, removing several cases of anonymous class instantiation to provide its functionality.

It's still possible to access static methods through $this-> so this is also non-breaking to consumers.

@tcarrio tcarrio requested a review from a team as a code owner October 25, 2022 17:09
@welcome
Copy link

welcome bot commented Oct 25, 2022

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

@tcarrio tcarrio mentioned this pull request Oct 25, 2022
@Nevay
Copy link
Contributor

Nevay commented Oct 25, 2022

IMO we should drop the trait and use Accessor directly.

@tcarrio
Copy link
Contributor Author

tcarrio commented Oct 25, 2022

I agree, generally the trait doesn't add anything other than passing arguments on to the Accessor, besides a couple cases where it's coercing types to be what's "expected". Accessor appears to want strings always.

Example:
Accessor::getFloat expects a string default value
EnvironmentVariablesTrait::getFloatFromEnvironment expects a float for the default value

When it comes to entirely static traits though, the benefits of this type of mixin behavior are somewhat lost. A single EnvironmentVariables class with static methods would instead provide all of these things- unless it's consumed as part of the public API directly such that Sdk should have public methods for retrieving environment variables.

So options:

  1. Make the trait's methods static
  2. Remove EnvironmentVariablesTrait entirely
  3. Provide a more friendly-typed EnvironmentVariables class with static methods

The largest impact in my opinion on this is whether anything using the trait does so not for internal private access, but public APIs. Namely, Sdk uses this trait, and it may be expected that those methods exist, which provide more friendly types (the EnvironmentVariablesTrait methods) and environment variable lookup behavior (the underlying Accessor).

@brettmc
Copy link
Collaborator

brettmc commented Oct 26, 2022

I'm in favour of option 3 - it doesn't need to be a trait, and I appreciate the typing and self-documenting that a class such as EnvironmentVariables would give.
I'm not aware of any code outside of our own which might be accessing env vars, but I could imagine 3rd party -contrib code could do in future.

…d access

utilizes Accessor and Variables under the hood to provide consistent default values
@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Merging #850 (d8189f1) into main (f677757) will decrease coverage by 0.05%.
The diff coverage is 86.56%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #850      +/-   ##
============================================
- Coverage     81.75%   81.69%   -0.06%     
+ Complexity     1947     1946       -1     
============================================
  Files           243      241       -2     
  Lines          5069     5080      +11     
============================================
+ Hits           4144     4150       +6     
- Misses          925      930       +5     
Flag Coverage Δ
7.4 81.17% <86.56%> (-0.06%) ⬇️
8.0 81.69% <86.56%> (-0.06%) ⬇️
8.1 81.83% <86.56%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
src/SDK/Resource/ResourceInfoFactory.php 60.00% <ø> (ø)
src/Contrib/OtlpGrpc/Exporter.php 92.00% <68.42%> (ø)
src/Contrib/OtlpHttp/Exporter.php 87.50% <75.00%> (-0.50%) ⬇️
...rc/SDK/Common/Environment/EnvironmentVariables.php 100.00% <100.00%> (ø)
src/SDK/Resource/Detectors/Environment.php 100.00% <100.00%> (ø)
src/SDK/SDK.php 100.00% <100.00%> (ø)
src/SDK/Trace/ExporterFactory.php 100.00% <100.00%> (ø)
src/SDK/Trace/SamplerFactory.php 100.00% <100.00%> (ø)
src/SDK/Trace/SpanLimitsBuilder.php 61.90% <100.00%> (ø)
src/SDK/Trace/SpanProcessorFactory.php 92.85% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f677757...d8189f1. Read the comment docs.

@brettmc brettmc merged commit 8798973 into open-telemetry:main Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants