Skip to content

optimize resource creation #1210

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

Merged
merged 3 commits into from
Jan 13, 2024
Merged

Conversation

brettmc
Copy link
Collaborator

@brettmc brettmc commented Jan 12, 2024

Profiling the SDK autoloader startup with xdebug, I could see that we re-detect resources multiple times which adds overhead.

  • instead of creating new resources from various factories, create them once from sdk autoloader and
    pass them to the factories (most of which already supported this).
  • cache header from OtlpUtil, to save a couple of constructor calls to Sdk detector
  • check env before php.ini for config, since this is the more popular approach and saves some cycles
  • note as a todo that we could save some further cycles around resource merging by not re-validating attributes

- instead of creating new resources from various factories, create them once from sdk autoloader and
pass them to the factories (most of which already supported this).
- check env before php.ini for config, since this is the more popular approach and saves some cycles
- cache header from OtlpUtil, to save a couple of calls to Sdk detector
@brettmc brettmc requested a review from a team January 12, 2024 03:18
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (a196c13) 84.08% compared to head (305fde9) 83.22%.
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1210      +/-   ##
============================================
- Coverage     84.08%   83.22%   -0.86%     
- Complexity     2237     2251      +14     
============================================
  Files           285      285              
  Lines          6359     6409      +50     
============================================
- Hits           5347     5334      -13     
- Misses         1012     1075      +63     
Flag Coverage Δ
7.4 81.90% <70.58%> (-0.86%) ⬇️
8.0 83.17% <70.58%> (-0.85%) ⬇️
8.1 83.30% <70.58%> (-0.85%) ⬇️
8.2 83.30% <70.58%> (-0.85%) ⬇️
8.3 83.30% <70.58%> (-0.85%) ⬇️

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

Files Coverage Δ
...ommon/Configuration/Resolver/CompositeResolver.php 100.00% <100.00%> (ø)
src/SDK/Logs/LoggerProviderFactory.php 85.71% <100.00%> (ø)
src/SDK/Metrics/MeterProviderFactory.php 76.92% <100.00%> (ø)
src/SDK/Resource/ResourceInfo.php 100.00% <ø> (ø)
src/SDK/SdkAutoloader.php 94.11% <100.00%> (+0.36%) ⬆️
src/Contrib/Otlp/OtlpUtil.php 61.53% <37.50%> (-38.47%) ⬇️

... and 13 files with indirect coverage changes


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 a196c13...305fde9. Read the comment docs.

@@ -62,6 +62,7 @@ public function serialize(): string
* resource, the value of the updating resource MUST be picked (even if the updated value is empty)
*
* @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/resource/sdk.md#merge
* @todo can we optimize this to avoid re-validating the attributes on merge?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Created an issue for this: #1212

@brettmc brettmc merged commit 3928b13 into open-telemetry:main Jan 13, 2024
@brettmc brettmc deleted the tune-resource-creation branch January 29, 2025 21:50
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