-
Notifications
You must be signed in to change notification settings - Fork 438
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
[EXPORTER] Async exporting for otlp grpc #2407
[EXPORTER] Async exporting for otlp grpc #2407
Conversation
… by using a modern malloc library.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2407 +/- ##
==========================================
- Coverage 87.12% 87.09% -0.02%
==========================================
Files 200 200
Lines 6109 6103 -6
==========================================
- Hits 5322 5315 -7
- Misses 787 788 +1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix.
It will take me several review passes to go over this change,
please find a first batch of comments for now.
exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_client.h
Outdated
Show resolved
Hide resolved
…tlp_grpc_exporter # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive work.
Please find here more comments / questions.
@marcalff It would be better if we can suppor a low version of gRPC. We have some project with old toolchains which can only use gRPC 1.33. |
Thanks for the clarification. Resolved then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, thanks for the fix.
I will not pretend I know grpc async in depth to fully appreciate the code here.
The patch looks good to me, based on my limited knowledge of this area.
Given the patch size, a second opinion is needed.
Added the please review label, for a second review. |
@@ -23,6 +23,8 @@ namespace exporter | |||
namespace otlp | |||
{ | |||
|
|||
class OtlpGrpcClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be moved to otlp_grpc_exporter_options.h
as a central place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users use the factory and otlp_grpc*_options.h
to create OTLP gRPC exporters, but I think users do not need to know the internal class OtlpGrpcClient
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @owent for yet another great work.
I had a minor question only.
…tlp_grpc_exporter
…porter' into async_exporting_for_otlp_grpc_exporter
Fixes #2406
Changes
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes