-
Notifications
You must be signed in to change notification settings - Fork 487
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
Add prometheus.receive_http
component to allow receiving metrics over HTTP
#3753
Conversation
50086b9
to
fa6739a
Compare
ebf9540
to
733aa95
Compare
936db14
to
f01609a
Compare
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.
PTAL - ready for review
go.mod
Outdated
@@ -171,7 +171,7 @@ require ( | |||
golang.org/x/time v0.3.0 | |||
google.golang.org/api v0.109.0 | |||
google.golang.org/grpc v1.52.3 | |||
google.golang.org/protobuf v1.28.1 | |||
google.golang.org/protobuf v1.30.0 |
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.
The update was needed so that I can use the protoadapt
package in api_test.go - see comments in that file for more details.
return err | ||
} | ||
|
||
buf, err := proto.Marshal(protoadapt.MessageV2Of(req)) |
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.
We need to use protoadapt.MessageV2Of
since the Agent repo uses the new V2 google.golang.org/protobuf library, while prometheus uses the V1 github.com/golang/protobuf library. Importing the V1 is a linter error.
In order to use the protoadapt
, I had to upgrade the google.golang.org/protobuf
to 1.30.0 - see go.mod
.
I don't think it's a problem since it's a minor version and we would eventually want to update this dependency anyway.
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.
Nice progress, this is getting close! Comments are mainly around documentation and naming, looking forward to seeing this land!
@@ -0,0 +1,392 @@ | |||
package api |
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.
Nice and readable testing! :)
docs/sources/flow/reference/components/prometheus.source.api.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/prometheus.source.api.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/prometheus.source.api.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/prometheus.source.api.md
Outdated
Show resolved
Hide resolved
}) | ||
} | ||
|
||
type Arguments struct { |
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.
(not a blocker, just putting some thoughts out, I'm on the fence on whether we should handle this or not)/
Right now, a user can pass a gRPC block (even if we don't explicitly call it out or document it). Should we provide an UnmarshalRiver method that validates against it and returns an error if it's being set? Should we create wrappers for http-only and grpc-only flavors of common/net
? This is related to what you're proposing on #3780 and the two issues we've opened upstream so let's discuss this a bit more.
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.
That would make things more user-friendly, however, it seems like a temporary workaround and the GRPC server will still start anyway...
I think we would ideally want a server that can have HTTP/GRPC only and also separate squash
-ed HTTP and GRPC blocks. That way, if someone provided GRPC config to this component, the river syntax check would give an error without any boilerplate code.
Co-authored-by: Paschalis Tsilias <tpaschalis@users.noreply.github.com>
Co-authored-by: Paschalis Tsilias <tpaschalis@users.noreply.github.com>
Co-authored-by: Paschalis Tsilias <tpaschalis@users.noreply.github.com>
prometheus.source.api
component to allow receiving metrics over HTTPprometheus.receive_http
component to allow receiving metrics over HTTP
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, nice!
PR Description
Adds a new
prometheus.receive_http
component to allow receiving metrics over HTTP.Which issue(s) this PR fixes
Fixes #2780
PR Checklist