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

grpc: factor out base classes for gRPC based drivers #276

Merged
merged 25 commits into from
Oct 7, 2024

Conversation

alltilla
Copy link
Member

@alltilla alltilla commented Sep 6, 2024

TODO:

  • fix macOS build
  • manually try out all the drivers
  • write news entries about the new options
  • follow-up using macros in the parser.h files in cfg-helper

For the reviewers:

  • It is not trivial which options are gRPC related and which are actual driver specific. I have made a classification myself and factored out what I thought to be common, but please think about it yourselves, while you are reviewing.
  • Some features that I classified as gRPC related were not implemented in some drivers, so I added them. Please double check if the newly added features make sense in the drivers.
  • Is it the best to statically link the grpc-common target? I thought that dynamic linking would be better but I received a lot of linker warnings about it not being portable. Have I missed something?
  • Is there a better way to merge two sets of keywords than defining a macro like GRPC_KEYWORDS? It messes up the cfg-helper :/ gcc -E and a bit of parsing improvement in axosyslog-cfg-helper will be able to handle this.

Copy link
Contributor

github-actions bot commented Sep 6, 2024

This Pull Request introduces config grammar changes

axoflow/8d8eab09d02508dfeddf1cdbc8fae5d783ef3489 -> alltilla/grpc-factor-out-common-classes

--- a/destination
+++ b/destination

 axosyslog-otlp(
+    keep-alive(
+        <empty>
+        max-pings-without-data(<nonnegative-integer>)
+        time(<nonnegative-integer>)
+        timeout(<nonnegative-integer>)
+    )
 )

 bigquery(
+    auth(
+        adc(<empty>)
+        alts(
+            <empty>
+            target-service-accounts(
+                <empty>
+                <string>
+            )
+        )
+        insecure(<empty>)
+        tls(
+            <empty>
+            ca-file(<string>)
+            cert-file(<string>)
+            key-file(<string>)
+        )
+    )
 )

 loki(
+    batch-bytes(<positive-integer>)
+    compression(<yesno>)
 )

 opentelemetry(
+    keep-alive(
+        <empty>
+        max-pings-without-data(<nonnegative-integer>)
+        time(<nonnegative-integer>)
+        timeout(<nonnegative-integer>)
+    )
 )

 syslog-ng-otlp(
+    keep-alive(
+        <empty>
+        max-pings-without-data(<nonnegative-integer>)
+        time(<nonnegative-integer>)
+        timeout(<nonnegative-integer>)
+    )
 )

Copy link
Contributor

@OverOrion OverOrion left a comment

Choose a reason for hiding this comment

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

Let me get back to you with answers to your questions / requests.
Overall a solid looking PR, huge kudos for doing the extraction 💯

modules/grpc/credentials/Makefile.am Show resolved Hide resolved
modules/grpc/metrics/CMakeLists.txt Show resolved Hide resolved
modules/grpc/common/grpc-dest-worker.cpp Outdated Show resolved Hide resolved
modules/grpc/common/grpc-dest.hpp Outdated Show resolved Hide resolved
modules/grpc/common/grpc-dest.hpp Outdated Show resolved Hide resolved
modules/grpc/common/grpc-source.cpp Outdated Show resolved Hide resolved
modules/grpc/common/grpc-source.cpp Outdated Show resolved Hide resolved
modules/grpc/common/grpc-source.hpp Outdated Show resolved Hide resolved
modules/grpc/common/grpc-source.hpp Outdated Show resolved Hide resolved
lib/merge-grammar.py Show resolved Hide resolved
@alltilla
Copy link
Member Author

diff --git a/modules/grpc/common/Makefile.am b/modules/grpc/common/Makefile.am
index 541ba9f05..5c2cd99c6 100644
--- a/modules/grpc/common/Makefile.am
+++ b/modules/grpc/common/Makefile.am
@@ -3,7 +3,7 @@ include modules/grpc/common/metrics/Makefile.am
 
 if ENABLE_GRPC
 
-noinst_LTLIBRARIES += modules/grpc/common/libgrpc-common.la
+lib_LTLIBRARIES += modules/grpc/common/libgrpc-common.la
 
 GRPC_COMMON_CFLAGS = \
   -I$(top_srcdir)/modules/grpc/common \

causes

*** Warning: Linking the shared library modules/grpc/loki/libloki.la against the loadable module
*** libgrpc-common.so is not portable!
  CXXLD    modules/grpc/otel/libotel_cpp.la
  CXXLD    modules/grpc/otel/libotel.la

*** Warning: Linking the shared library modules/grpc/otel/libotel.la against the loadable module
*** libgrpc-common.so is not portable!
  CXXLD    modules/grpc/bigquery/libbigquery_cpp.la
  CXXLD    modules/grpc/bigquery/libbigquery.la

*** Warning: Linking the shared library modules/grpc/bigquery/libbigquery.la against the loadable module
*** libgrpc-common.so is not portable!

@alltilla alltilla force-pushed the grpc-factor-out-common-classes branch 2 times, most recently from faedca4 to 121be0c Compare September 16, 2024 13:58
alltilla added a commit to alltilla/axosyslog that referenced this pull request Sep 17, 2024
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla alltilla force-pushed the grpc-factor-out-common-classes branch from 121be0c to 02559f1 Compare September 17, 2024 15:29
alltilla added a commit to alltilla/axosyslog that referenced this pull request Sep 18, 2024
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla alltilla force-pushed the grpc-factor-out-common-classes branch from 02559f1 to a0eb426 Compare September 18, 2024 11:25
OverOrion
OverOrion previously approved these changes Sep 20, 2024
@MrAnno
Copy link
Member

MrAnno commented Sep 24, 2024

I think we shouldn't introduce more dynamic libraries around the gRPC drivers, we already have one (static linking seems more than okay in this specific case).

alltilla added a commit to alltilla/axosyslog that referenced this pull request Sep 24, 2024
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla alltilla force-pushed the grpc-factor-out-common-classes branch from a0eb426 to d6d37f5 Compare September 24, 2024 12:13
@alltilla
Copy link
Member Author

Fixed one review finding and rebased to main.

@MrAnno
Copy link
Member

MrAnno commented Sep 26, 2024

Let's ask @mitzkia to check the compatibility of this PR next week (manually or in an automated way).

alltilla added a commit to alltilla/axosyslog that referenced this pull request Oct 4, 2024
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla alltilla force-pushed the grpc-factor-out-common-classes branch from d6d37f5 to 72e64bc Compare October 4, 2024 08:30
@alltilla
Copy link
Member Author

alltilla commented Oct 4, 2024

rebased to main

@alltilla
Copy link
Member Author

alltilla commented Oct 7, 2024

@MrAnno I had to add some fixup commits that came from the manual E2E tests, could you kindly check them? Thanks!

Copy link
Member

@MrAnno MrAnno left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

modules/grpc/common/grpc-source.cpp Outdated Show resolved Hide resolved
alltilla added a commit to alltilla/axosyslog that referenced this pull request Oct 7, 2024
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla alltilla force-pushed the grpc-factor-out-common-classes branch from 65d3bba to 6b20819 Compare October 7, 2024 09:31
Signed-off-by: Attila Szakacs <[email protected]>
With this a grammar file can load additional grammar files
for declarations to use.

Signed-off-by: Attila Szakacs <[email protected]>
Signed-off-by: Attila Szakacs <[email protected]>
Signed-off-by: Attila Szakacs <[email protected]>
Signed-off-by: Attila Szakacs <[email protected]>
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla alltilla force-pushed the grpc-factor-out-common-classes branch from 6b20819 to 9413815 Compare October 7, 2024 09:35
@alltilla alltilla requested a review from MrAnno October 7, 2024 09:40
@MrAnno MrAnno merged commit e241431 into axoflow:main Oct 7, 2024
22 checks passed
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.

3 participants