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

Symfony Bundle compatibility updates for beta #169

Merged
merged 9 commits into from
Jun 28, 2023

Conversation

wadjei
Copy link
Contributor

@wadjei wadjei commented Jun 19, 2023

  • Change configuration to be based on SpanExporterFactory definitions instead of Exporter factories - this facilitates the injection of TransportInterfaces rather than the client etc.
  • Remove references to obsolete, removed Jaeger and NewRelic exporters
  • Updated unit and integration tests

@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Merging #169 (f541db2) into main (1fed588) will increase coverage by 20.14%.
The diff coverage is 23.80%.

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

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##              main     #169       +/-   ##
============================================
+ Coverage     7.82%   27.97%   +20.14%     
- Complexity     242      584      +342     
============================================
  Files           25       49       +24     
  Lines          971     2295     +1324     
============================================
+ Hits            76      642      +566     
- Misses         895     1653      +758     
Flag Coverage Δ
7.4 38.47% <23.80%> (+20.82%) ⬆️
8.0 30.45% <23.80%> (+20.76%) ⬆️
8.1 9.81% <ø> (ø)
8.2 7.83% <ø> (ø)

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

Impacted Files Coverage Δ
...telSdkBundle/DependencyInjection/Configuration.php 0.00% <0.00%> (ø)
...SdkBundle/DependencyInjection/OtelSdkExtension.php 0.00% <0.00%> (ø)
...fony/src/OtelBundle/HttpKernel/RequestListener.php 77.34% <100.00%> (ø)
...src/OtelSdkBundle/Debug/TraceableSpanProcessor.php 100.00% <100.00%> (ø)
...ymfony/src/OtelSdkBundle/Trace/ExporterFactory.php 100.00% <100.00%> (ø)

... and 19 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 1fed588...21c510f. Read the comment docs.

Copy link
Collaborator

@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.

Looking good! Just a couple of minor comments from me - newrelic and DSN-based config. There were more newrelic mentions in the code that I didn't comment on, could you search and remove?

edit: if the tests are now passing for symfony, they can be re-enabled (in .github/workflows)

@wadjei
Copy link
Contributor Author

wadjei commented Jun 26, 2023

edit: if the tests are now passing for symfony, they can be re-enabled (in .github/workflows)

the workflow seems to be happy with Symfony tests re-enabled, but having rebased on main, there's a lot of new failures in AWS, Logs/Monolog and Instrumentation seemingly unconnected with my changes

@wadjei wadjei requested a review from brettmc June 26, 2023 12:20
@brettmc
Copy link
Collaborator

brettmc commented Jun 26, 2023

there's a lot of new failures in AWS, Logs/Monolog and Instrumentation

Actively fixing those, most are OK now if you re-base. In any case, definitely not caused by you and I don't think a blocker for this PR.

@brettmc
Copy link
Collaborator

brettmc commented Jun 27, 2023

@wadjei let's get this merged. There's a failing style check for symfony that should be an easy fix (make style locally), then if it goes green I'll merge it.

* Change configuration to be based on SpanExporterFactory definitions instead of Exporter factories - this facilitates the injection of TransportInterfaces rather than the client etc.
* Remove references to obsolete, removed Jaeger and NewRelic exporters
* Updated unit and integration tests
includes:
- vendor/phpstan/phpstan-phpunit/extension.neon
- vendor/phpstan/phpstan-symfony/extension.neon
- vendor/jangregor/phpstan-prophecy/extension.neon
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to avoid adding this config but without it phpstan cannot deal with prophecy mocks

@brettmc brettmc merged commit af42807 into open-telemetry:main Jun 28, 2023
@wadjei
Copy link
Contributor Author

wadjei commented Jul 3, 2023

@brettmc thanks for reviewing and merging... A few minor things left over

  • when running the test workflow, there were differences in the static analysis stages between PHP 7.4/8.0 and 8.1/8.2 which could not be resolved in one without breaking the other. I came up with a minor re-write to get around that - shall I open a PR for it?
  • The namespace of the Symfony bundle now differs between the monorepo's psr-4 definition and that of the bundle itself. Should we refactor the bundle so it matches in all cases? (OpenTelemetry\Contrib\Symfony looks more correct to me now)
  • Finally the readme of the bundle no longer offers any info for anyone trying it out. is it time to restore the old one?

@brettmc
Copy link
Collaborator

brettmc commented Jul 3, 2023

@wadjei

  • yes please, submit a new PR to fix those static analysis issues
  • I agree, moving it somewhere is needed. I don't have a better idea than Contrib
  • yes, let's restore the old if it's still fairly accurate

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