Skip to content

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

Merged
MrAnno merged 25 commits intoaxoflow:mainfrom
alltilla:grpc-factor-out-common-classes
Oct 7, 2024
Merged

grpc: factor out base classes for gRPC based drivers#276
MrAnno merged 25 commits intoaxoflow:mainfrom
alltilla:grpc-factor-out-common-classes

Conversation

@alltilla
Copy link
Member

@alltilla alltilla commented Sep 6, 2024 •
edited
Loading

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.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2024 •
edited
Loading

This Pull Request introduces config grammar changes

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

Details 
--- 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>)
+    )
 )

@alltilla alltilla force-pushed the grpc-factor-out-common-classes branch from 20aa005 to 7e51356 Compare September 6, 2024 17:37
alltilla
alltilla commented Sep 9, 2024
View reviewed changes
alltilla
alltilla commented Sep 9, 2024
View reviewed changes
alltilla
alltilla commented Sep 9, 2024
View reviewed changes
alltilla
alltilla commented Sep 9, 2024
View reviewed changes
OverOrion
OverOrion reviewed Sep 9, 2024
View reviewed changes
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 💯

@alltilla
Copy link
Member Author

alltilla commented Sep 10, 2024 

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
@alltilla
news: add entries for axoflow#276
02559f1 
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
@alltilla
news: add entries for axoflow#276
a0eb426 
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
View reviewed changes
MrAnno
MrAnno reviewed Sep 24, 2024
View reviewed changes
@MrAnno
Copy link
Contributor

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
@alltilla
news: add entries for axoflow#276
d6d37f5 
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

alltilla commented Sep 24, 2024

Fixed one review finding and rebased to main.

@MrAnno
Copy link
Contributor

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
@alltilla
news: add entries for axoflow#276
72e64bc 
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 added a commit to alltilla/axosyslog that referenced this pull request Oct 7, 2024
@alltilla
news: add entries for axoflow#276
41fe827 
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
alltilla added 16 commits October 7, 2024 11:34
@alltilla
grpc: create SourceDriver and SourceWorker base classes
2378f51 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
otel: derive SourceDriver and SourceWorker from grpc base classes
146cae6 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
grpc: factor out common grpc keywords for parsers
b498093 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
merge-grammar: factor out locate_file()
8d3d9a9 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
merge-grammar: factor out main()
50271e3 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
merge-grammar: make including more general
e12bce7 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
merge-grammar: collect blocks once
420a53e 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
merge-grammar: factor out print_preprocessed_grammar()
29974d9 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
merge-grammar: support REQUIRE command
9e2629a 
With this a grammar file can load additional grammar files
for declarations to use.

Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
grpc: create a common grammar file
7c64172 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
otel: use grpc-grammar.ym
32f79d5 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
loki: use grpc-grammar.ym
66b5895 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
bigquery: use grpc-grammar.ym
841df10 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
grpc/otel: handle credentials creation error
32f50f8 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
grpc: replace std::pair<x,y>{} with std::make_pair()
295f02e 
Signed-off-by: Attila Szakacs <[email protected]>
@alltilla
news: add entries for axoflow#276
9413815 
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
fekete-robert pushed a commit to axoflow/axosyslog-core-docs that referenced this pull request Nov 8, 2024
[4.9][loki] Adds batch-bytes and compression options
8e720e7 
axoflow/axosyslog#276
fekete-robert pushed a commit to axoflow/axosyslog-core-docs that referenced this pull request Nov 8, 2024
[4.9][bigquery] Adds grpc authentication
950df3a 
axoflow/axosyslog#276
@HofiOne HofiOne mentioned this pull request Nov 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MrAnno MrAnno MrAnno approved these changes

@OverOrion OverOrion OverOrion left review comments

@bshifter bshifter Awaiting requested review from bshifter

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alltilla @MrAnno @OverOrion