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

Added Sdk::isDeveloperModeEnabled() helper. #1144

Conversation

ChrisLightfootWild
Copy link
Contributor

Use case in mind:

  • allow extra functionality to be hooked during developer mode

Alternative:

  • create additional packages that can be added as composer dev-dependencies, which can register via autoloading in the usual way

Copy link

welcome bot commented Nov 17, 2023

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.

Copy link

codecov bot commented Nov 19, 2023

Codecov Report

Merging #1144 (bf21fd4) into main (d6c4c89) will decrease coverage by 0.02%.
The diff coverage is 71.42%.

❗ Current head bf21fd4 differs from pull request most recent head d997392. Consider uploading reports for the commit d997392 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1144      +/-   ##
============================================
- Coverage     84.28%   84.27%   -0.02%     
- Complexity     2193     2197       +4     
============================================
  Files           282      282              
  Lines          6224     6231       +7     
============================================
+ Hits           5246     5251       +5     
- Misses          978      980       +2     
Flag Coverage Δ
7.4 82.91% <71.42%> (-0.02%) ⬇️
8.0 84.20% <71.42%> (-0.02%) ⬇️
8.1 84.34% <71.42%> (-0.02%) ⬇️
8.2 84.34% <71.42%> (-0.02%) ⬇️
8.3 84.34% <71.42%> (-0.02%) ⬇️

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

Files Coverage Δ
src/SDK/Sdk.php 92.30% <71.42%> (-7.70%) ⬇️

Continue to review full report in Codecov by Sentry.

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

@brettmc
Copy link
Collaborator

brettmc commented Nov 20, 2023

Can you elaborate on what sort of extra functionality you're thinking of?

src/SDK/Sdk.php Outdated
return Configuration::getBoolean(Variables::OTEL_PHP_DEVELOPER_MODE_ENABLED);
}

try {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should only be enabled by variable (and default to off). Is there any benefit other than convenience to also checking InstalledVersions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, purely the convenience. I will go with your lead here, will remove this dependency/complexity.

@ChrisLightfootWild
Copy link
Contributor Author

Can you elaborate on what sort of extra functionality you're thinking of?

It was along the lines of hooking things which may only be available as a dev-dependency, without having to split that into a separate package.

The split may be additional overhead, but happy either way. There may not be a strong enough use-case for this and that is fine too!

@ChrisLightfootWild
Copy link
Contributor Author

ChrisLightfootWild commented Nov 21, 2023

Can you elaborate on what sort of extra functionality you're thinking of?

With #1045 in mind, to be able to hook additional classes which may or may not be present.

With that particular example, something akin to the below but without the overhead of the hook call where it is not required:

if ($devMode && isset(\Illuminate\Foundation\Console\ServeCommand::$passthroughVariables)) {
    foreach ($_ENV as $key => $value) {
        if (str_starts_with($key, 'OTEL_')) {
            \Illuminate\Foundation\Console\ServeCommand::$passthroughVariables[] = $key;
        }
    }
}

@brettmc
Copy link
Collaborator

brettmc commented Nov 21, 2023

Gotcha. My understanding of the observer API (mostly from reading https://www.datadoghq.com/blog/engineering/php-8-observability-baked-right-in/#target-functions-and-methods) is that it's very low overhead for methods that aren't called. There's no issue with observing non-existent methods, either (eg if the class is not installed). So, I'm not particularly worried about the overhead of extra hooks.

Even so, I think you could wrap the hook with an if class_exists(DevDependency::class) and that would achieve what you're after?

@ChrisLightfootWild ChrisLightfootWild deleted the experimental/dev-mode branch November 23, 2023 08:05
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.

2 participants