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

Adds experimental service.instance.id resource detector #1309

Merged
merged 21 commits into from
Jun 15, 2024

Conversation

matt-hensley
Copy link
Contributor

@matt-hensley matt-hensley commented May 13, 2024

Adds experimental service.instance.id resource detector per experimental semantic conventions.

This detector has been added to the list of well known detectors with the ID service and can be enabled via the OTEL_PHP_DETECTORS environment variable.

@matt-hensley matt-hensley requested a review from a team May 13, 2024 15:07
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.23%. Comparing base (39f81a3) to head (7729036).

Current head 7729036 differs from pull request most recent head c416219

Please upload reports for the commit c416219 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1309      +/-   ##
============================================
- Coverage     74.32%   74.23%   -0.10%     
- Complexity     2491     2493       +2     
============================================
  Files           353      354       +1     
  Lines          7135     7144       +9     
============================================
  Hits           5303     5303              
- Misses         1832     1841       +9     
Flag Coverage Δ
8.1 ?
8.2 ?
8.3 ?
8.4 74.23% <100.00%> (-0.06%) ⬇️

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

Files Coverage Δ
src/SDK/Resource/Detectors/Service.php 100.00% <100.00%> (ø)
src/SDK/Resource/ResourceInfoFactory.php 97.67% <100.00%> (+0.11%) ⬆️

... and 6 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 39f81a3...c416219. Read the comment docs.

@brettmc
Copy link
Collaborator

brettmc commented May 13, 2024

@matt-hensley looking good. phan and deptrac issues are config-related:

  • add uuid to .phan/config.php/directory_list
  • add uuid to deptrac.yaml and allow SDK to depend on it

@matt-hensley matt-hensley requested a review from brettmc May 14, 2024 01:07
deptrac.yaml Show resolved Hide resolved
@brettmc
Copy link
Collaborator

brettmc commented May 14, 2024

Couple of minor nits from me, but otherwise looks good.

Co-authored-by: Brett McBride <[email protected]>
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@matt-hensley matt-hensley requested a review from brettmc May 14, 2024 02:30
composer.json Show resolved Hide resolved
@ChrisLightfootWild ChrisLightfootWild dismissed their stale review May 14, 2024 14:29

composer changes were addressed.

@brettmc
Copy link
Collaborator

brettmc commented May 17, 2024

The failure is because we just moved from EnvironmentVariables test trait to OpenTelemetry\Tests\TestState - might have been a mis-merge for that to not come across when you merged in main...

@brettmc
Copy link
Collaborator

brettmc commented May 17, 2024

@matt-hensley you can ignore the psalm errors. I'm aware of them: mockery/mockery#1421
If we get desperate, a workaround is to go back a minor version of mockery.

@brettmc
Copy link
Collaborator

brettmc commented May 21, 2024

@matt-hensley if you merge in main now, those psalm errors should be gone.

@brettmc brettmc merged commit 6bf422c into open-telemetry:main Jun 15, 2024
9 checks passed
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.

4 participants