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.

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

Fix: correct the value of otelcol_exporter_send_failed_requests #4629

Merged

Conversation

ralphgj
Copy link
Contributor

@ralphgj ralphgj commented Jan 4, 2022

The metric of otelcol_exporter_send_failed_requests defined in obsreportconfig.go used view.Count() as the aggregation.

	errorNumberView := &view.View{
		Name:        obsmetrics.ExporterPrefix + "send_failed_requests",
		Description: "number of times exporters failed to send requests to the destination",
		Measure:     obsmetrics.ExporterFailedToSendSpans,
		Aggregation: view.Count(),
	}

In obsreport_exporter.go, while numFailedToSend(obsmetrics.ExporterFailedToSendSpans) equals 0, it will also increase count of the metric. The actual meaning of otelcol_exporter_send_failed_requests is the total count of requests that sent to destination but not the failed requests.

func (exp *Exporter) recordMetrics(ctx context.Context, numSent, numFailedToSend int64, sentMeasure, failedToSendMeasure *stats.Int64Measure) {
	if obsreportconfig.Level() == configtelemetry.LevelNone {
		return
	}
	// Ignore the error for now. This should not happen.
	_ = stats.RecordWithTags(ctx, exp.mutators, sentMeasure.M(numSent), failedToSendMeasure.M(numFailedToSend))
}

@ralphgj ralphgj requested a review from a team as a code owner January 4, 2022 10:35
@ralphgj ralphgj force-pushed the fix-send_failed_requests-metrics branch from a002c6e to cba873f Compare January 4, 2022 11:45
@bogdandrutu
Copy link
Member

@alolita I remember this metric was added to help some AWS health extension. Does this fix affects you?

@ralphgj ralphgj force-pushed the fix-send_failed_requests-metrics branch from cba873f to f2a3a4a Compare January 5, 2022 03:36
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #4629 (5e27ca4) into main (f13a011) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4629      +/-   ##
==========================================
+ Coverage   90.44%   90.46%   +0.02%     
==========================================
  Files         181      181              
  Lines       10598    10611      +13     
==========================================
+ Hits         9585     9599      +14     
+ Misses        789      788       -1     
  Partials      224      224              
Impacted Files Coverage Δ
obsreport/obsreport_exporter.go 94.44% <100.00%> (+0.44%) ⬆️
obsreport/obsreporttest/obsreporttest.go 95.58% <100.00%> (+1.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f13a011...5e27ca4. Read the comment docs.

@bogdandrutu bogdandrutu enabled auto-merge (squash) January 5, 2022 18:42
@bogdandrutu bogdandrutu merged commit d552222 into open-telemetry:main Jan 5, 2022
@ralphgj ralphgj deleted the fix-send_failed_requests-metrics branch January 7, 2022 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants