-
Notifications
You must be signed in to change notification settings - Fork 32
NETOBSERV-584: Support for writing logs to Loki (distributor) using gRPC #1086
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1086 +/- ##
==========================================
+ Coverage 65.67% 65.97% +0.30%
==========================================
Files 121 121
Lines 8934 9065 +131
==========================================
+ Hits 5867 5981 +114
- Misses 2735 2747 +12
- Partials 332 337 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=232d236 make set-flp-image |
@leandroberetta: This pull request references NETOBSERV-584 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
c2a742e
to
53fc1fe
Compare
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=fecd9f7 make set-flp-image |
pkg/api/write_loki.go
Outdated
MaxSendMsgSize int `yaml:"maxSendMsgSize,omitempty" json:"maxSendMsgSize,omitempty" doc:"maximum message size the client can send"` | ||
KeepAlive string `yaml:"keepAlive,omitempty" json:"keepAlive,omitempty" doc:"keep alive interval"` | ||
KeepAliveTimeout string `yaml:"keepAliveTimeout,omitempty" json:"keepAliveTimeout,omitempty" doc:"keep alive timeout"` | ||
TLS *GRPCTLSConfig `yaml:"tls,omitempty" json:"tls,omitempty" doc:"TLS configuration"` |
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.
Same here, we may reuse ClientConfig
that contains TLSConfig
?
https://github.com/prometheus/common/blob/v0.66.1/config/http_config.go#L1113
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.
Just checked that, surely that's doable, but prom's TLS config contains more advanced settings (e.g. min / max versions) which we would need to support in our implementation in the loki-client, if we want to be consistent with the proposed settings. Or we would need to reuse prom's implementation but I'm not sure they have grpc in the go-client. So perhaps having our own struct is good enough for now, after all.
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.
wait, actually, we do also have another TLS struct of our own: api.ClientTLS
That one could be reused easily, no ?
/hold
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.
Hi, I've just seen this comment, actually, I did the work to reuse the prom's tls config. Also, I've made the necessary changes in the loki-client-go at netobserv/loki-client-go#4
@jotak the one you mentioned is used for Kafka and it seemed to be more simple than the prom one, for example, for now, communicating with the distributor in grpc mode, I need to configure mTLS which I don't see that is possible with the kafka one.
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.
oh, cool, that should be even better
go.mod
Outdated
|
||
replace github.com/vmware/go-ipfix => github.com/jotak/go-ipfix v0.0.0-20250708115123-407c539ea101 | ||
|
||
replace github.com/netobserv/loki-client-go => github.com/leandroberetta/loki-client-go v0.0.0-20250924145641-591b5499691d |
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.
you can now remove that as this was merged
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.
/lgtm
thanks @leandroberetta !
[edit] actually, last-minute comment: #1086 (comment)
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
e18125e
to
0e34623
Compare
0e34623
to
d01e386
Compare
@leandroberetta: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
Support for writing logs to Loki (distributor) using gRPC
Ref: https://issues.redhat.com/browse/NETOBSERV-584
Dependencies
netobserv/loki-client-go#3
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.
To run a perfscale test, comment with:
/test flp-node-density-heavy-25nodes