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

[WIP] foundation for building with contrib + sockets library - main repo part I #871

Closed
wants to merge 135 commits into from

Conversation

maxgolov
Copy link
Contributor

@maxgolov maxgolov commented Jun 22, 2021

Changes

PR implements the necessary plumbing for fluentd / fluentbit exporters that live in contrib repo. Since we already have a library for basic cross-plat sockets support (used by HTTP server), I refactored that library to support TCP, UDP and Unix Domain sockets. Subsequently, fluentd exporter uses that library to talk to end-point over TCP, UDP or Unix Domain.

To explain the structure:

  • /ext/include/opentelemetry/ext/net/common - network library, socket client, socket server. Reusable, (still) used by HTTP server. Now with more sockets support, i.e. in addition to TCP (that was already there) - I added UDP and Unix Domain.
  • /ext/test/sockets - cross-plat tests for the sockets.
  • CMakeLists.txt - that allows to build with contrib specified via local directory, or by FetchContent from contrib repo.

Subsequently, contrib repo part II relies on some generic / general purpose code that lives in the main, e.g. in order to avoid reimplementing the same socket library again. The code is Apache License 2.0, all copyright OpenTelemetry Authors. No need to borrow any alternate BSD or Boost -licensed network library.

@lalitb - corresponding PR for the fluentd exporter is in the contrib repo is here:
open-telemetry/opentelemetry-cpp-contrib#42

Clean-up a few typos
Reword the last sentence
@maxgolov maxgolov requested a review from a team June 22, 2021 05:35
@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #871 (2168e09) into main (2fd7bd2) will decrease coverage by 0.63%.
The diff coverage is 80.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #871      +/-   ##
==========================================
- Coverage   95.50%   94.88%   -0.62%     
==========================================
  Files         156      158       +2     
  Lines        6620     6810     +190     
==========================================
+ Hits         6322     6461     +139     
- Misses        298      349      +51     
Impacted Files Coverage Δ
api/include/opentelemetry/common/string_util.h 100.00% <ø> (ø)
api/test/common/kv_properties_test.cc 100.00% <ø> (ø)
api/test/common/string_util_test.cc 100.00% <ø> (ø)
...nclude/opentelemetry/ext/net/common/socket_tools.h 80.10% <80.10%> (ø)
api/include/opentelemetry/common/kv_properties.h 92.23% <0.00%> (-6.66%) ⬇️
sdk/test/resource/resource_test.cc 93.46% <0.00%> (-3.48%) ⬇️
sdk/src/resource/resource.cc 80.00% <0.00%> (-2.60%) ⬇️
... and 2 more

@maxgolov maxgolov marked this pull request as draft June 25, 2021 00:09
@@ -121,6 +121,86 @@
"type": "BOOL"
}
]
}
]
},
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 will remove this. This is just an illustration how to build with contrib from Visual Studio. I'll move this into separate document.

@@ -14,6 +14,8 @@ using namespace OPENTELEMETRY_NAMESPACE;

using namespace opentelemetry::exporter::etw;
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 will revert this, as I submitted these changes in a separate PR #882

@lalitb
Copy link
Member

lalitb commented Jul 8, 2021

I would review it more thoroughly once this is ready for review, the general comments I have for now are:

  • socket library - Very nicely done and this would be definitely useful for applications requiring cross-platform socker lib, we need to discuss whether the main repo is the right place for this. While I see the possibility of using the socket library internally for Jaeger thrift implementation, but not exposing it as api/header to external systems. As an example, we do have an internal HTTP client and (test) server implementation, but this can't be used from external systems. The main repo should provide interfaces for opentelemetry-api, opentelemetry-sdk, various core exporters, and not more than that. It can be in a separate repo so that both fluentd and Jaeger can use from there, or else let's keep it within fluentd exporter for now?

  • For the building with Contrib repo - This has been discussed in community meetings and some good discussions in issue Allow to build SDK + contrib repo via BUILD_CONTRIB build option #713, would be good to have wider feedback from @open-telemetry/cpp-approvers and @open-telemetry/cpp-maintainers.

@lalitb
Copy link
Member

lalitb commented Sep 15, 2021

Closing this PR. Plan is to consolidate these changes with fluentd exporter in the contrib repo.

@lalitb lalitb closed this Sep 15, 2021
@reyang reyang deleted the maxgolov/fluentd_exporter branch March 6, 2022 20:52
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