Skip to content

Conversation

@HeenaBansal20
Copy link
Contributor

@HeenaBansal20 HeenaBansal20 commented May 14, 2025

This PR is to add the support for baggage propagation in Instana propagator plus unit tests.

This branch is followup PR of #365.
Once this PR is merged , I'll rebase this feature branch on master.

@HeenaBansal20 HeenaBansal20 requested a review from a team as a code owner May 14, 2025 04:10
@HeenaBansal20 HeenaBansal20 marked this pull request as draft May 14, 2025 04:10
@HeenaBansal20
Copy link
Contributor Author

Make all is green locally. can we try re-trigger the CI 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
Nothing to modify in lock file
Writing lock file
Installing dependencies from lock file (including require-dev)
Nothing to install, update or remove
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".
 4/4 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

   1) src/InstanaPropagator.php (ordered_imports)

Fixed 1 of 4 files in 0.341 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/blalNf, vendor/bin/phan]
   analyze ████████████████████████████████████████████████████████████ 100.0% 1029MB/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...

703 / 703...

Analyzing files...

░░░

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

Checks took 5.10 seconds and used 81.158MB of memory
Psalm was able to infer types for 99.0588% 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.
 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 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 Context Propagator (OpenTelemetry\Tests\Propagation\Instana\Unit\InstanaContextPropagator)
 ✔ Fields  4 ms
 ✔ Inject empty  4 ms
 ✔ Inject invalid context  2 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  1 ms
 ✔ Extract context with no span headers  1 ms
 ✔ Extract sampled context with baggage with String·sampled·value  2 ms
 ✔ Extract sampled context with baggage with Boolean(lower·string)·sampled·value  1 ms
 ✔ Extract sampled context with baggage with Boolean(upper·string)·sampled·value  1 ms
 ✔ Extract sampled context with baggage with Boolean(camel·string)·sampled·value  1 ms
 ✔ Extract sampled context with baggage but instana propagator with String·sampled·value  1 ms
 ✔ Extract sampled context with baggage but instana propagator with Boolean(lower·string)·sampled·value  1 ms
 ✔ Extract sampled context with baggage but instana propagator with Boolean(upper·string)·sampled·value  1 ms
 ✔ Extract sampled context with baggage but instana propagator with Boolean(camel·string)·sampled·value  1 ms
 ✔ Baggage inject  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.022, Memory: 6.00 MB

OK (39 tests, 59 assertions)

@HeenaBansal20 HeenaBansal20 marked this pull request as ready for review May 15, 2025 10:08
@codecov
Copy link

codecov bot commented May 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.72%. Comparing base (3f003f3) to head (099e2a2).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #370      +/-   ##
============================================
+ Coverage     81.68%   81.72%   +0.03%     
- Complexity     1741     1745       +4     
============================================
  Files           133      134       +1     
  Lines          7307     7322      +15     
============================================
+ Hits           5969     5984      +15     
  Misses         1338     1338              
Flag Coverage Δ
Aws 92.59% <ø> (ø)
Context/Swoole 0.00% <ø> (ø)
Instrumentation/CakePHP 20.40% <ø> (ø)
Instrumentation/CodeIgniter 73.55% <ø> (ø)
Instrumentation/Doctrine 92.72% <ø> (ø)
Instrumentation/ExtAmqp 88.48% <ø> (ø)
Instrumentation/ExtRdKafka 86.11% <ø> (ø)
Instrumentation/Guzzle 69.13% <ø> (ø)
Instrumentation/HttpAsyncClient 78.04% <ø> (ø)
Instrumentation/IO 70.68% <ø> (ø)
Instrumentation/Laravel 63.91% <ø> (ø)
Instrumentation/MongoDB 74.28% <ø> (ø)
Instrumentation/MySqli 95.81% <ø> (ø)
Instrumentation/PDO 94.21% <ø> (ø)
Instrumentation/Psr14 76.47% <ø> (ø)
Instrumentation/Psr15 89.15% <ø> (ø)
Instrumentation/Psr16 97.50% <ø> (ø)
Instrumentation/Psr18 77.46% <ø> (ø)
Instrumentation/Psr3 67.01% <ø> (ø)
Instrumentation/Psr6 97.61% <ø> (ø)
Instrumentation/Slim 86.11% <ø> (ø)
Instrumentation/Symfony 84.74% <ø> (ø)
Instrumentation/Yii 77.50% <ø> (ø)
Logs/Monolog 100.00% <ø> (ø)
Propagation/Instana 98.11% <100.00%> (ø)
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 Δ
src/Propagation/Instana/src/InstanaPropagator.php 98.11% <100.00%> (ø)

... and 1 file 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 3f003f3...099e2a2. Read the comment docs.

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

@HeenaBansal20 HeenaBansal20 force-pushed the feature/add-baggage-propagation-in-instana branch from 847e8e7 to fcd8659 Compare May 15, 2025 16:49
@HeenaBansal20
Copy link
Contributor Author

@brettmc @ChrisLightfootWild , Please review this followup PR on Instana propagator.

@ChrisLightfootWild ChrisLightfootWild requested a review from a team May 18, 2025 14:19
Copy link
Contributor

@brettmc brettmc left a comment

Choose a reason for hiding this comment

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

This is not the right approach. If users want to do baggage propagation, they can use opentelemetry's MultiTextMapPropagator, and pass it an Instana and Baggage propagator. That's the way it's meant to be used, for example:

$propagator = new MultiTextMapPropagator(InstantPropagator::getInstance(), BaggagePropagator::getInstance();

It should also work with env-based autoloading, eg OTEL_PROPAGATORS=instana,baggage

So, Instana propagators should only concern themselves with the implementation of Instana headers propagation.

@brettmc
Copy link
Contributor

brettmc commented May 20, 2025

Looks much better now. Some minor comments, and CI linting is unhappy (make style locally should fix this)

@HeenaBansal20 HeenaBansal20 requested a review from brettmc May 20, 2025 02:30
@HeenaBansal20
Copy link
Contributor Author

@brettmc , I see that downloading the package directly using composer gives error since there is no release version tagged to this package . is it something which will be created by you on this package?

~# composer require open-telemetry/opentelemetry-propagation-instana gives below error.

n 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>...]

@brettmc brettmc merged commit da5f909 into open-telemetry:main May 20, 2025
130 of 136 checks passed
@brettmc
Copy link
Contributor

brettmc commented May 20, 2025

https://packagist.org/packages/open-telemetry/opentelemetry-propagation-instana - can you run some sanity checks on dev-main here, and report back? Then, I'll tag an initial version.

@HeenaBansal20
Copy link
Contributor Author

https://packagist.org/packages/open-telemetry/opentelemetry-propagation-instana - can you run some sanity checks on dev-main here, and report back? Then, I'll tag an initial version.

Sure @brettmc when you do sanity checks .. does Otel-php has any requirements/checklist to be followed?

@brettmc
Copy link
Contributor

brettmc commented May 20, 2025

when you do sanity checks .. does Otel-php has any requirements/checklist to be followed?

No. Just satisfy yourself that you can download it, and it works as expected against a real application (eg participating in the middle of a distributed trace)

@HeenaBansal20
Copy link
Contributor Author

@brettmc , I tested the dev-main branch of contrib-instana-propagator package and distributed tracing works as expected. I can see all different services coming under one trace using instana native headers.
I see there are few typo errors in readme, I have created the PR to correct readme details.
Please approve and push the tag for the package.
Thank you so much for your quick support and guidance.

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.

3 participants