otlp: add gRPC and HTTP input components #3895
Conversation
|
Requires #3893 merged first. |
mmatczuk
force-pushed
the
mmt/otelinput
branch
6 times, most recently
from
6602493 to
6894c8d
Compare
internal/impl/otlp/input.go
Outdated
| shutSig *shutdown.Signaller | ||
| } | ||
|
|
||
| func makeOTLPInput(mgr *service.Resources, rateLimit string) otlpInput { |
There was a problem hiding this comment.
Why makeOTLPInput instead of following the Go convention of newOTLPInput?
There was a problem hiding this comment.
Also, given the type uses pointer receivers for its functions, would it be better to follow the convention of making this function return a pointer to indicate that it has pointer semantics?
There was a problem hiding this comment.
The thing is this is only for embedding - the pointer receiver works on addresses embedded in another struct.
I can sure rename it to new.
internal/impl/otlp/input.go
Outdated
|
|
||
| // maybeWaitForAccess blocks until the rate limiter grants access or the | ||
| // context/shutdown signals. If no rate limit is configured, it returns | ||
| // immediately. It must be called before calling [sendMessage]. |
There was a problem hiding this comment.
Should this be sendMessageBatch?
| // immediately. It must be called before calling [sendMessage]. | |
| // immediately. It must be called before calling [sendMessageBatch]. |
internal/impl/otlp/input.go
Outdated
| err = rerr | ||
| } | ||
| if err != nil { | ||
| o.log.Errorf("Rate limit error: %v\n", err) |
There was a problem hiding this comment.
| o.log.Errorf("Rate limit error: %v\n", err) | |
| o.log.Errorf("Rate limit error: %v", err) |
I don't think we need the newline here, or was this intentional?
internal/impl/otlp/input.go
Outdated
| } | ||
| } | ||
|
|
||
| const gracefulShutdownTimeout = 5 * time.Second |
There was a problem hiding this comment.
minor (non-blocking): given this is only used in the input_grpc.go file, would it be worth moving it to the top of that file as opposed to in here at the bottom?
There was a problem hiding this comment.
Moved above Close.
| ) | ||
|
|
||
| type grpcInputConfig struct { | ||
| Address string |
There was a problem hiding this comment.
Do these fields need to be exported?
There was a problem hiding this comment.
Fair point, those are exported fields of not exported type but it's on purpose for config type. It can make your life easier with tests or in separate package otlp_test.
internal/impl/otlp/input_grpc.go
Outdated
| ) | ||
|
|
||
| // Parse gRPC-specific config | ||
| conf.Address, err = pConf.FieldString(giFieldAddress) |
There was a problem hiding this comment.
Minor (non-blocking): Very subjective so feel free to ignore, but these seem like a good candidate for inlining, ie:
if conf.Address, err = pConf.FieldString(giFieldAddress); err != nil {
return nil, err
}
internal/impl/otlp/input_grpc.go
Outdated
|
|
||
| s.maybeWaitForAccess(ctx) | ||
|
|
||
| traces := req.Traces() |
There was a problem hiding this comment.
Minor (non-blocking): I don't see us using traces variable later, perhaps just call req.Traces().SpanCount() instead of assigning to a variable?
mmatczuk
force-pushed
the
mmt/otelinput
branch
3 times, most recently
from
1746df2 to
c6d96f2
Compare
Add OpenTelemetry Protocol (OTLP) input components supporting both gRPC and HTTP transports for ingesting traces, logs, and metrics. - Add otlp_grpc input on default port 4317 - Add otlp_http input on default port 4318 with /v1/traces, /v1/logs, and /v1/metrics endpoints - Convert OTLP protobuf format to Redpanda OTEL v1 protobuf messages - Unbatch export requests into individual messages (one per span/log/metric) - Support optional rate limiting via rate limit resources - Add TLS configuration for both transports - Add TCP socket configuration (SO_REUSEADDR, SO_REUSEPORT)
mmatczuk
force-pushed
the
mmt/otelinput
branch
from
c6d96f2 to
6c0be2e
Compare
Add OpenTelemetry Protocol (OTLP) input components supporting both gRPC
and HTTP transports for ingesting traces, logs, and metrics.
/v1/metrics endpoints