Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Separate OTLP HTTP and GRPC Exporters #399

Merged
merged 2 commits into from
Apr 30, 2023

Conversation

Sherlouk
Copy link
Contributor

@Sherlouk Sherlouk commented Apr 7, 2023

Closes #389

Created three targets: Common, Grpc, and HTTP. The latter two are also products.
The default product (same name as previous) will use Grpc to aide in backwards compatibility.

Clients using HTTP will have to change imports, but given the experimental nature of the current implementation I don't believe this to be a major problem. However, if others disagree we could make the default product include both Grpc and HTTP and then have separate GRPC and HTTP targets for those who specifically only want one.

Updated sample code which proves we're able to drop the NIO and GRPC dependencies. Not done full benchmarks, but locally the sample product does build quicker.

No other changes beyond separation and imports. Basic compile tests done, but not a full integraiton.

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.10 🎉

Comparison is base (a7004d0) 68.04% compared to head (cb8b704) 68.14%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
+ Coverage   68.04%   68.14%   +0.10%     
==========================================
  Files         253      253              
  Lines       11820    11820              
==========================================
+ Hits         8043     8055      +12     
+ Misses       3777     3765      -12     
Impacted Files Coverage Δ
...TelemetryProtocolCommon/common/EnvVarHeaders.swift 97.36% <ø> (ø)
...s/OpenTelemetryProtocolCommon/common/Headers.swift 100.00% <ø> (ø)
...metryProtocolCommon/common/OtlpConfiguration.swift 100.00% <ø> (ø)
...elemetryProtocolCommon/logs/LogRecordAdapter.swift 100.00% <ø> (ø)
...ryProtocolCommon/trace/utils/TraceProtoUtils.swift 22.22% <ø> (ø)
...enTelemetryProtocolGrpc/logs/OtlpLogExporter.swift 92.30% <ø> (ø)
...emetryProtocolGrpc/metric/OtlpMetricExporter.swift 93.33% <ø> (ø)
...elemetryProtocolGrpc/trace/OtlpTraceExporter.swift 93.18% <ø> (ø)
...etryProtocolGrpc/trace/OtlpTraceJsonExporter.swift 88.57% <ø> (ø)
...porters/OpenTelemetryProtocolHttp/HTTPClient.swift 93.75% <ø> (ø)
... and 8 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@nachoBonafonte nachoBonafonte left a comment

Choose a reason for hiding this comment

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

Looks really good to me, thanks a lot for your contribution

@nachoBonafonte
Copy link
Member

nachoBonafonte commented Apr 10, 2023

Hi Sherlouk, after merging a previous PR (#331 ) it brought some conflicts with your code, could you resolve it?, it might need some code in both exporters now. I will merge after done

@Sherlouk
Copy link
Contributor Author

Will try and find some time to rebase, little strapped for time though now as I'll be on holiday for the next week 👍

Have tested it locally though and am happy with functionality.

@Sherlouk Sherlouk force-pushed the separate-http-grpc branch from 7062c38 to 6cbd1be Compare April 28, 2023 21:58
Just some recent changes which I hadn't accounted for
@nachoBonafonte nachoBonafonte merged commit a85c8c9 into open-telemetry:main Apr 30, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate OTLP Exporter HTTP from gRPC
2 participants