Skip to content

Conversation

@HeenaBansal20
Copy link
Contributor

Which problem is this PR solving?
This PR adds a vendor specific propagator for the vendor specific trace correlation headers used by Instana.

Short description of the changes
Add Instana propagator plus unit tests.

@HeenaBansal20 HeenaBansal20 requested a review from a team as a code owner May 9, 2025 03:46
@welcome
Copy link

welcome bot commented May 9, 2025

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.

@codecov
Copy link

codecov bot commented May 9, 2025

Codecov Report

Attention: Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.72%. Comparing base (e201e11) to head (9d1d3c5).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...Propagation/Instana/src/InstanaMultiPropagator.php 98.11% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #365      +/-   ##
============================================
- Coverage     82.17%   81.72%   -0.46%     
+ Complexity     1870     1745     -125     
============================================
  Files           137      134       -3     
  Lines          7822     7322     -500     
============================================
- Hits           6428     5984     -444     
+ Misses         1394     1338      -56     
Flag Coverage Δ
Aws 92.59% <ø> (ø)
Context/Swoole 0.00% <ø> (ø)
Instrumentation/CakePHP 20.40% <ø> (+0.13%) ⬆️
Instrumentation/CodeIgniter 73.55% <ø> (-0.22%) ⬇️
Instrumentation/Curl ?
Instrumentation/Doctrine 92.72% <ø> (-0.05%) ⬇️
Instrumentation/ExtAmqp 88.48% <ø> (-0.78%) ⬇️
Instrumentation/ExtRdKafka 86.11% <ø> (-0.13%) ⬇️
Instrumentation/Guzzle 69.13% <ø> (-0.38%) ⬇️
Instrumentation/HttpAsyncClient 78.04% <ø> (-0.27%) ⬇️
Instrumentation/IO 70.68% <ø> (ø)
Instrumentation/Laravel 63.91% <ø> (-0.20%) ⬇️
Instrumentation/MongoDB 74.28% <ø> (ø)
Instrumentation/MySqli 95.81% <ø> (ø)
Instrumentation/OpenAIPHP ?
Instrumentation/PDO 94.21% <ø> (-0.02%) ⬇️
Instrumentation/Psr14 76.47% <ø> (-0.68%) ⬇️
Instrumentation/Psr15 89.15% <ø> (-0.26%) ⬇️
Instrumentation/Psr16 97.50% <ø> (-0.07%) ⬇️
Instrumentation/Psr18 77.46% <ø> (-0.32%) ⬇️
Instrumentation/Psr3 67.01% <ø> (ø)
Instrumentation/Psr6 97.61% <ø> (-0.06%) ⬇️
Instrumentation/Slim 86.11% <ø> (-0.20%) ⬇️
Instrumentation/Symfony 84.74% <ø> (-0.20%) ⬇️
Instrumentation/Yii 77.50% <ø> (-0.19%) ⬇️
Logs/Monolog 100.00% <ø> (ø)
Propagation/Instana 98.11% <98.11%> (?)
Propagation/ServerTiming 100.00% <ø> (ø)
Propagation/TraceResponse 100.00% <ø> (ø)
ResourceDetectors/Azure 91.66% <ø> (ø)
ResourceDetectors/Container 93.02% <ø> (ø)
Sampler/RuleBased 33.51% <ø> (ø)
Shims/OpenTracing 92.45% <ø> (ø)
Symfony 87.81% <ø> (ø)
Utils/Test 87.53% <ø> (ø)

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

Files with missing lines Coverage Δ
...Propagation/Instana/src/InstanaMultiPropagator.php 98.11% <98.11%> (ø)

... and 28 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 e201e11...9d1d3c5. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@HeenaBansal20
Copy link
Contributor Author

HeenaBansal20 commented May 9, 2025

@bobstrecansky , Thank you for triggering the CI for this PR. However, the CI did not run for my project because I forgot to include the project in the GitHub workflow. Could you please retrigger the CI actions?

Additionally, I’ve noticed that some unrelated projects, such as Instrumentation/Psr15, Instrumentation/Psr16, and others, are failing their CI checks. Since these are not dependent on this PR, could you advise on how such failures should be handled?

@ChrisLightfootWild
Copy link
Contributor

Additionally, I’ve noticed that some unrelated projects, such as Instrumentation/Psr15, Instrumentation/Psr16, and others, are failing their CI checks. Since these are not dependent on this PR, could you advise on how such failures should be handled?

You can ignore the unrelated failures here. 👍

@HeenaBansal20
Copy link
Contributor Author

HeenaBansal20 commented May 13, 2025

I am not able to see these CI issues with local execution , However CI reports it .
Is there any difference how github actions run than the local execution.

fork/opentelemetry-php-contrib# PROJECT=Propagation/Instana PHP_VERSION=8.3 make phan
docker compose run --rm -w /usr/src/myapp/src/Propagation/Instana php env XDEBUG_MODE=off env PHAN_DISABLE_XDEBUG_WARN=1 vendor/bin/phan
[debug] Checking PHAN_ALLOW_XDEBUG
[debug] Because xdebug was installed, Phan will restart.
[debug] The Xdebug extension is loaded (3.4.2) xdebug.mode=off
[debug] Process restarting (PHAN_ALLOW_XDEBUG=internal|3.4.2|1|*|*)
[debug] Running: [/usr/local/bin/php, -n, -c, /tmp/odfhpf, vendor/bin/phan]
   analyze ████████████████████████████████████████████████████████████ 100.0% 1027MB/1041MB
[debug] Restarted process exited 0

@HeenaBansal20
Copy link
Contributor Author

HeenaBansal20 commented May 14, 2025

Hmm. I wonder why make all didn’t catch those errors. Obviously missed something with psalm override attribute too for psalm.
Okay, hopefully I got it right this time. Can we try the tests again?

# PROJECT=Propagation/Instana PHP_VERSION=8.3 make all
docker compose run --rm -w /usr/src/myapp/src/Propagation/Instana php env XDEBUG_MODE=off composer update --no-plugins
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 2 updates, 0 removals
  - Upgrading open-telemetry/api (1.2.3 => 1.3.0)
  - Upgrading open-telemetry/sdk (1.3.0 => 1.4.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 2 updates, 0 removals
  - Downloading open-telemetry/api (1.3.0)
  - Downloading open-telemetry/sdk (1.4.0)
  - Upgrading open-telemetry/api (1.2.3 => 1.3.0): Extracting archive
  - Upgrading open-telemetry/sdk (1.3.0 => 1.4.0): Extracting archive
Package php-http/message-factory is abandoned, you should avoid using it. Use psr/http-factory instead.
Generating autoload files
79 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
No security vulnerability advisories found.
docker compose run --rm -w /usr/src/myapp/src/Propagation/Instana php env XDEBUG_MODE=off PHP_CS_FIXER_IGNORE_ENV=1 vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.php --using-cache=no -vvv
PHP CS Fixer 3.75.0 Persian Successor by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.3.19
Running analysis on 1 core sequentially.
You can enable parallel runner and speed up the analysis! Please see usage docs for more information.
Loaded config default from ".php-cs-fixer.php".
 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


Fixed 0 of 3 files in 0.345 seconds, 18.00 MB memory used
docker compose run --rm -w /usr/src/myapp/src/Propagation/Instana php env XDEBUG_MODE=off composer validate --no-plugins
./composer.json is valid
docker compose run --rm -w /usr/src/myapp/src/Propagation/Instana php env XDEBUG_MODE=off env PHAN_DISABLE_XDEBUG_WARN=1 vendor/bin/phan
[debug] Checking PHAN_ALLOW_XDEBUG
[debug] Because xdebug was installed, Phan will restart.
[debug] The Xdebug extension is loaded (3.4.2) xdebug.mode=off
[debug] Process restarting (PHAN_ALLOW_XDEBUG=internal|3.4.2|1|*|*)
[debug] Running: [/usr/local/bin/php, -n, -c, /tmp/lKjjBi, vendor/bin/phan]
   analyze ████████████████████████████████████████████████████████████ 100.0% 1027MB/1041MB
[debug] Restarted process exited 0
docker compose run --rm -w /usr/src/myapp/src/Propagation/Instana php env XDEBUG_MODE=off vendor/bin/psalm --threads=1 --no-cache --php-version=8.3

JIT acceleration: ON

Target PHP version: 8.3 (set by CLI argument).

Running on PHP 8.3.19.


Scanning files...

701 / 701...

Analyzing files...

░░

------------------------------
                              
       No errors found!       
                              
------------------------------
14 other issues found.
You can display them with --show-info=true
------------------------------
Psalm can automatically fix 2 of these issues.
Run Psalm again with 
--alter --issues=MissingParamType --dry-run
to see what it can fix.
------------------------------

Checks took 5.61 seconds and used 80.722MB of memory
Psalm was able to infer types for 100% of the codebase
docker compose run --rm -w /usr/src/myapp/src/Propagation/Instana php env XDEBUG_MODE=off vendor/bin/phpstan analyse --memory-limit=256M
Note: Using configuration file /usr/src/myapp/src/Propagation/Instana/phpstan.neon.dist.
 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                                                        
 [OK] No errors                                                                                                         
                                                                                                                        

docker compose run --rm -w /usr/src/myapp/src/Propagation/Instana php env XDEBUG_MODE=off vendor/bin/phpunit --testdox --colors=always
PHPUnit 9.6.23 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.19
Configuration: /usr/src/myapp/src/Propagation/Instana/phpunit.xml.dist

Instana Multi Propagator (OpenTelemetry\Tests\Propagation\Instana\Unit\InstanaMultiPropagator)
 ✔ Fields  4 ms
 ✔ Inject empty  10 ms
 ✔ Inject invalid context  3 ms
 ✔ Inject sampled context  1 ms
 ✔ Inject non sampled context  1 ms
 ✔ Inject sampled context when other traceflags set  1 ms
 ✔ Extract context with lowercase headers  2 ms
 ✔ Extract context with no span headers  1 ms
 ✔ Extract sampled context with String·sampled·value  1 ms
 ✔ Extract sampled context with Boolean(lower·string)·sampled·value  1 ms
 ✔ Extract sampled context with Boolean(upper·string)·sampled·value  1 ms
 ✔ Extract sampled context with Boolean(camel·string)·sampled·value  1 ms
 ✔ Extract non sampled context with String·sampled·value  1 ms
 ✔ Extract non sampled context with Boolean(lower·string)·sampled·value  1 ms
 ✔ Extract non sampled context with Boolean(upper·string)·sampled·value  1 ms
 ✔ Extract non sampled context with Boolean(camel·string)·sampled·value  1 ms
 ✔ Extract default sampled context with null·sampled·value  1 ms
 ✔ Extract default sampled context with empty·sampled·value  1 ms
 ✔ Extract invalid sampled context with wrong·sampled·value·1  1 ms
 ✔ Extract invalid sampled context with wrong·sampled·value·2  1 ms
 ✔ Extract context with sampled no trace and span headers  1 ms
 ✔ Extract context with no trace and span headers  1 ms
 ✔ Extract and inject  1 ms
 ✔ Extract empty trace id  1 ms
 ✔ Extract leftpad spand id  1 ms
 ✔ Invalid trace id  1 ms
 ✔ Invalid trace id size  1 ms
 ✔ Extract empty span id  1 ms
 ✔ Invalid span id  1 ms
 ✔ Invalid span id size  1 ms

Time: 00:00.027, Memory: 6.00 MB

@HeenaBansal20
Copy link
Contributor Author

Many thanks @brettmc for approving the PR. I see that CI checks for the Instana in successful for all supported PHP versions ? Is this PR good to merge ?

Copy link
Contributor

@ChrisLightfootWild ChrisLightfootWild left a comment

Choose a reason for hiding this comment

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

Left some final formatting bits I spotted, but that can always be done in your follow-up PR.

Comment on lines +9 to +10
}
Registry::registerTextMapPropagator(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
Registry::registerTextMapPropagator(
}
Registry::registerTextMapPropagator(

}

if ($spanId && strlen($spanId) < 16) {
$spanId = str_pad($spanId, 16, '0', STR_PAD_LEFT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$spanId = str_pad($spanId, 16, '0', STR_PAD_LEFT);
$spanId = str_pad($spanId, 16, '0', STR_PAD_LEFT);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Will be addressed in the followup PR.
#370

Comment on lines +116 to +119
return (new NonRecordingSpan($spanContext))
->storeInContext($context);

} elseif (!$spanContext->isValid()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return (new NonRecordingSpan($spanContext))
->storeInContext($context);
} elseif (!$spanContext->isValid()) {
return (new NonRecordingSpan($spanContext))
->storeInContext($context);
} elseif (!$spanContext->isValid()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Will be addressed in the followup PR.
#370

@brettmc brettmc merged commit 3f003f3 into open-telemetry:main May 14, 2025
129 of 136 checks passed
@HeenaBansal20
Copy link
Contributor Author

HeenaBansal20 commented May 15, 2025

@brettmc @ChrisLightfootWild , where this Instana propagation package will be hosted on packagist.org.
I see that opentelemetry-php-contrib package is abandoned.
https://packagist.org/packages/open-telemetry/opentelemetry-php-contrib
and downloading the package directly using
~# composer require open-telemetry/opentelemetry-propagation-instana gives below error.

In PackageDiscoveryTrait.php line 383:
                                                                                                                                                                                                                                                    
  Could not find a matching version of package open-telemetry/opentelemetry-propagation-instana. Check the package spelling, your version constraint and that the package is available in a stability which matches your minimum-stability (stable  
  ).                                                                                                                                                                                                                                                
                                                                                                                                                                                                                                                    

require [--dev] [--dry-run] [--prefer-source] [--prefer-dist] [--prefer-install PREFER-INSTALL] [--fixed] [--no-suggest] [--no-progress] [--no-update] [--no-install] [--no-audit] [--audit-format AUDIT-FORMAT] [--update-no-dev] [-w|--update-with-dependencies] [-W|--update-with-all-dependencies] [--with-dependencies] [--with-all-dependencies] [--ignore-platform-req IGNORE-PLATFORM-REQ] [--ignore-platform-reqs] [--prefer-stable] [--prefer-lowest] [-m|--minimal-changes] [--sort-packages] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--apcu-autoloader-prefix APCU-AUTOLOADER-PREFIX] [--] [<packages>...]

Thanks!

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