Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "context" | ||
| "fmt" | ||
| "net/http" | ||
| "sync/atomic" | ||
| "time" | ||
|
|
||
| "go.opentelemetry.io/otel/attribute" | ||
| "go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc" | ||
| "go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp" | ||
| "go.opentelemetry.io/otel/metric" | ||
| metricsdk "go.opentelemetry.io/otel/sdk/metric" | ||
|
|
||
| "github.com/redpanda-data/benthos/v4/public/service" | ||
| ) | ||
|
|
||
| func otlpMetricsSpec() *service.ConfigSpec { | ||
| return service.NewConfigSpec(). | ||
| Summary("Send metrics to an https://opentelemetry.io/docs/collector/[Open Telemetry collector^]."). | ||
| Fields( | ||
| service.NewStringField("service"). | ||
| Default("benthos"). | ||
| Description("The name of the service in metrics."), |
There was a problem hiding this comment.
Field names should be defined as constants with a component-prefix convention (e.g., omFieldService, omFieldHTTP, etc.) rather than used as string literals. This applies to "service" here and in newOtlpMetrics ("service", "http", "grpc", "tags").
See project Go patterns:
Field names are always defined as constants with a component-prefix convention
<componentAbbrev>Field<Name>
Don't put field names as string literals. Do use constants
|
|
||
| func newOtlpMetrics(conf *service.ParsedConfig, _ *service.Logger) (*otlpMetrics, error) { | ||
| serviceName, err := conf.FieldString("service") | ||
| if err != nil { |
There was a problem hiding this comment.
The 30*time.Second timeout is a magic number and a hardcoded duration. Per project patterns, it should be a named constant at minimum, and ideally exposed as a YAML-configurable field.
See project Go patterns:
Name all numeric constants. Every literal number in logic must have a clear meaning through a named constant or variable.
Every time-related value (timeouts, backoffs, intervals, retry delays) must be exposed as a YAML-configurable field. Do not hardcode durations.
|
|
||
| func TestMetricsCounter(t *testing.T) { | ||
| m := newTestMetrics(t) | ||
| t.Cleanup(func() { require.NoError(t, m.Close(t.Context())) }) |
There was a problem hiding this comment.
t.Context() returns a canceled context during t.Cleanup() execution, which will cause m.Close() to fail if the OTel provider's Shutdown respects the context. Use context.Background() instead. This applies to all 7 t.Cleanup calls in this file.
Use
t.Context()for test contexts. Exception: int.Cleanup()functions, usecontext.Background()becauset.Context()is already canceled during cleanup.
|
Commits Review
|
- Extracted common collector parsing logic to `collectorListFields` and `parseCollectors`. - Simplified resource creation with `newResource` helper. - Renamed `oltp` to `otlpTracer` for clarity. - Updated tests to use new spec and config parsing functions.
Implement new metrics exporter for OpenTelemetry collectors, supporting both gRPC and HTTP protocols.
mmatczuk
force-pushed
the
mmt/otel_metrics
branch
from
7de3d6c to
05a7ae5
Compare
|
Commits Review LGTM |
2 participants
No description provided.