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

Add DsnParser + Otlp/HttpEndpointResolver and remove dependency on nyholm/dsn #767

Merged
merged 33 commits into from
Jul 18, 2022

Conversation

tidal
Copy link
Member

@tidal tidal commented Jul 16, 2022

This PR removes the dependency on the nyholm/dsn package by introducing dedicated DsnParser + Otlp/HttpEndpointResolver components.
The DSN we use in the Trace/ExporterFactory (and probably other factories in the future) uses a protocol=type+scheme pattern to identify different exporters. This is not supported by nyholm/dsn, so there had to be custom logic in the first place.
The other place where the package was used was resolving the OTLP HTTP endpoints according to the spec. This logic has been used into a dedicated class, which can be used for exporters of other signals in the future.

  • Adds Dsn, Dsn/Parser and Dsn/Factory.
  • Adds Otlp/HttpEndpointResolver
  • Removes dependency on nyhol/dsn

(Note: There were some outdated assumptions in the OTLPHttpExporterTest which dated back to before the introduction of signal specific Env Vars. These have just been removed for now. The OTLExporter tests need some rework anyway, which will be provided in a future PR.)

related:

@codecov
Copy link

codecov bot commented Jul 16, 2022

Codecov Report

Merging #767 (e05f9ae) into main (7f2e974) will increase coverage by 0.86%.
The diff coverage is 97.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #767      +/-   ##
============================================
+ Coverage     78.66%   79.53%   +0.86%     
- Complexity     1348     1409      +61     
============================================
  Files           156      160       +4     
  Lines          3314     3479     +165     
============================================
+ Hits           2607     2767     +160     
- Misses          707      712       +5     
Flag Coverage Δ
7.4 79.53% <97.16%> (+0.86%) ⬆️
8.0 79.60% <97.16%> (+0.86%) ⬆️
8.1 79.60% <97.16%> (+0.86%) ⬆️

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

Impacted Files Coverage Δ
src/SDK/Common/Dsn/Dsn.php 90.90% <90.90%> (ø)
src/Contrib/OtlpHttp/Exporter.php 94.00% <100.00%> (+1.27%) ⬆️
src/SDK/Common/Dsn/Factory.php 100.00% <100.00%> (ø)
src/SDK/Common/Dsn/Parser.php 100.00% <100.00%> (ø)
src/SDK/Common/Otlp/HttpEndpointResolver.php 100.00% <100.00%> (ø)
src/SDK/Trace/ExporterFactory.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f2e974...e05f9ae. Read the comment docs.

@tidal tidal added the blocked this issue is blocked by another issue. label Jul 16, 2022
@tidal
Copy link
Member Author

tidal commented Jul 16, 2022

Phan error should be resolved, when #768 is merged.

@Nevay
Copy link
Contributor

Nevay commented Jul 16, 2022

enqueue/dsn supports scheme extensions / their use case is very similar to ours. I think we should use ConnectionFactoryFactory as orientation for our implementation.

@tidal tidal removed the blocked this issue is blocked by another issue. label Jul 17, 2022
@brettmc
Copy link
Collaborator

brettmc commented Jul 18, 2022

looks good to me, pending Nevay's comment about enqueue/dsn ?

@tidal
Copy link
Member Author

tidal commented Jul 18, 2022

enqueue/dsn supports scheme extensions / their use case is very similar to ours. I think we should use ConnectionFactoryFactory as orientation for our implementation.

I guess you mean the ExporterFactory. I can look into that.

@tidal tidal merged commit 5a64c0d into open-telemetry:main Jul 18, 2022
@tidal tidal deleted the exporter branch July 18, 2022 06:18
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.

None yet

3 participants