Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Log dropped metric count when exporting from PRW exporter #3323

Closed
wants to merge 1 commit into from
Closed

Log dropped metric count when exporting from PRW exporter #3323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions exporter/prometheusremotewriteexporter/exporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/gogo/protobuf/proto"
"github.com/golang/snappy"
"github.com/prometheus/prometheus/prompb"
"go.uber.org/zap"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/confighttp"
Expand All @@ -51,10 +52,11 @@ type PRWExporter struct {
concurrency int
userAgentHeader string
clientSettings *confighttp.HTTPClientSettings
logger *zap.Logger
}

// NewPRWExporter initializes a new PRWExporter instance and sets fields accordingly.
func NewPRWExporter(cfg *Config, buildInfo component.BuildInfo) (*PRWExporter, error) {
func NewPRWExporter(cfg *Config, buildInfo component.BuildInfo, logger *zap.Logger) (*PRWExporter, error) {
sanitizedLabels, err := validateAndSanitizeExternalLabels(cfg.ExternalLabels)
if err != nil {
return nil, err
Expand All @@ -76,6 +78,7 @@ func NewPRWExporter(cfg *Config, buildInfo component.BuildInfo) (*PRWExporter, e
userAgentHeader: userAgentHeader,
concurrency: cfg.RemoteWriteQueue.NumConsumers,
clientSettings: &cfg.HTTPClientSettings,
logger: logger,
}, nil
}

Expand Down Expand Up @@ -158,7 +161,8 @@ func (prwe *PRWExporter) PushMetrics(ctx context.Context, md pdata.Metrics) erro
errs = append(errs, exportErrors...)
}

if dropped != 0 {
if dropped > 0 {
prwe.logger.Info("Prometheus remote write dropped metrics", zap.Int("dropped_count", dropped))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can potentially flood the logs. We shouldn't be doing this. See https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#logging

I believe all exporters record dropped metrics, which presumably is sufficient for observing the Collector. If we need a different way observe dropped data we need to explain why and we would prefer it to be uniform for all exporters, not special just for Prometheus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metrics would work in this case. I opened the PR due to my lack of awareness of the builtin metrics. I'll close this.

return consumererror.Combine(errs)
}

Expand Down
9 changes: 5 additions & 4 deletions exporter/prometheusremotewriteexporter/exporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/prometheus/prometheus/prompb"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/component/componenttest"
Expand Down Expand Up @@ -110,7 +111,7 @@ func Test_NewPRWExporter(t *testing.T) {
cfg.ExternalLabels = tt.externalLabels
cfg.Namespace = tt.namespace
cfg.RemoteWriteQueue.NumConsumers = 1
prwe, err := NewPRWExporter(cfg, tt.buildInfo)
prwe, err := NewPRWExporter(cfg, tt.buildInfo, zap.NewNop())

if tt.returnErrorOnCreate {
assert.Error(t, err)
Expand Down Expand Up @@ -191,7 +192,7 @@ func Test_Start(t *testing.T) {
cfg.RemoteWriteQueue.NumConsumers = 1
cfg.HTTPClientSettings = tt.clientSettings

prwe, err := NewPRWExporter(cfg, tt.buildInfo)
prwe, err := NewPRWExporter(cfg, tt.buildInfo, zap.NewNop())
assert.NoError(t, err)
assert.NotNil(t, prwe)

Expand Down Expand Up @@ -333,7 +334,7 @@ func runExportPipeline(ts *prompb.TimeSeries, endpoint *url.URL) []error {
Version: "1.0",
}
// after this, instantiate a CortexExporter with the current HTTP client and endpoint set to passed in endpoint
prwe, err := NewPRWExporter(cfg, buildInfo)
prwe, err := NewPRWExporter(cfg, buildInfo, zap.NewNop())
if err != nil {
errs = append(errs, err)
return errs
Expand Down Expand Up @@ -591,7 +592,7 @@ func Test_PushMetrics(t *testing.T) {
Description: "OpenTelemetry Collector",
Version: "1.0",
}
prwe, nErr := NewPRWExporter(cfg, buildInfo)
prwe, nErr := NewPRWExporter(cfg, buildInfo, zap.NewNop())
require.NoError(t, nErr)
require.NoError(t, prwe.Start(context.Background(), componenttest.NewNopHost()))
err := prwe.PushMetrics(context.Background(), *tt.md)
Expand Down
2 changes: 1 addition & 1 deletion exporter/prometheusremotewriteexporter/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func createMetricsExporter(_ context.Context, params component.ExporterCreatePar
return nil, errors.New("invalid configuration")
}

prwe, err := NewPRWExporter(prwCfg, params.BuildInfo)
prwe, err := NewPRWExporter(prwCfg, params.BuildInfo, params.Logger)
if err != nil {
return nil, err
}
Expand Down