Add ingoing and outgoing counts to processorhelper #10910

djaglowski · 2024-08-19T19:27:14Z

Description

Implements ingoing and outgoing counts as described in #10708.

codecov · 2024-08-19T19:36:29Z

Codecov Report

Attention: Patch coverage is 92.20779% with 6 lines in your changes missing coverage. Please review.

Project coverage is 92.21%. Comparing base (35024cf) to head (96867af).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
processor/processorhelper/logs.go	77.77%	1 Missing and 1 partial ⚠️
processor/processorhelper/metrics.go	77.77%	1 Missing and 1 partial ⚠️
processor/processorhelper/traces.go	77.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10910      +/-   ##
==========================================
- Coverage   92.21%   92.21%   -0.01%     
==========================================
  Files         414      414              
  Lines       19718    19792      +74     
==========================================
+ Hits        18183    18251      +68     
- Misses       1165     1168       +3     
- Partials      370      373       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

djaglowski · 2024-09-03T13:16:40Z

Any thoughts on this, @open-telemetry/collector-contrib-approvers?

codeboten

Thanks @djaglowski, this looks good overall, just one question around setting the meter provider

codeboten · 2024-09-06T17:44:24Z

processor/processorhelper/logs.go

@@ -39,12 +40,25 @@ func NewLogsProcessor(
 		return nil, errors.New("nil logsFunc")
 	}

+	if set.MeterProvider == nil {
+		set.MeterProvider = noop.NewMeterProvider()


i wouldn't expect this to be needed here, can you confirm whether it is?

You're right, it was a problem with a particular test, not something that needs to be addressed here.

codeboten · 2024-09-06T17:45:33Z

processor/processorhelper/metrics.go

@@ -39,12 +40,25 @@ func NewMetricsProcessor(
 		return nil, errors.New("nil metricsFunc")
 	}

+	if set.MeterProvider == nil {
+		set.MeterProvider = noop.NewMeterProvider()


same question as above

Removed for all three signals.

processor/processorhelper/metrics_test.go

processor/processorhelper/traces_test.go

djaglowski

I've rebased and resolved the conflict.

djaglowski · 2024-09-06T18:36:20Z

processor/processorhelper/logs.go

@@ -39,12 +40,25 @@ func NewLogsProcessor(
 		return nil, errors.New("nil logsFunc")
 	}

+	if set.MeterProvider == nil {
+		set.MeterProvider = noop.NewMeterProvider()


You're right, it was a problem with a particular test, not something that needs to be addressed here.

djaglowski · 2024-09-06T19:07:20Z

A contrib test failed, but I think it expected with this change:

=== Failed
=== FAIL: . TestFilterMetricProcessorTelemetry (0.00s)
make[3]: *** [../../Makefile.Common:128: test] Error 1
    generated_component_telemetry_test.go:55: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/processor/filterprocessor/generated_component_telemetry_test.go:55
        	            				/tmp/opentelemetry-collector-contrib/processor/filterprocessor/metrics_test.go:423
        	Error:      	Not equal: 
make[2]: *** [Makefile:187: processor/filterprocessor] Error 2
make[1]: *** [Makefile:127: gotest] Error 2
make: *** [Makefile:260: check-contrib] Error 2
        	            	expected: 1
        	            	actual  : 3
        	Test:       	TestFilterMetricProcessorTelemetry

=== FAIL: . TestFilterMetricProcessorTelemetry (re-run 1) (0.00s)
    generated_component_telemetry_test.go:55: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/processor/filterprocessor/generated_component_telemetry_test.go:55
        	            				/tmp/opentelemetry-collector-contrib/processor/filterprocessor/metrics_test.go:423
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 3
        	Test:       	TestFilterMetricProcessorTelemetry

codeboten · 2024-09-06T21:27:31Z

A contrib test failed, but I think it expected with this change:

=== Failed
=== FAIL: . TestFilterMetricProcessorTelemetry (0.00s)
make[3]: *** [../../Makefile.Common:128: test] Error 1
    generated_component_telemetry_test.go:55: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/processor/filterprocessor/generated_component_telemetry_test.go:55
        	            				/tmp/opentelemetry-collector-contrib/processor/filterprocessor/metrics_test.go:423
        	Error:      	Not equal: 
make[2]: *** [Makefile:187: processor/filterprocessor] Error 2
make[1]: *** [Makefile:127: gotest] Error 2
make: *** [Makefile:260: check-contrib] Error 2
        	            	expected: 1
        	            	actual  : 3
        	Test:       	TestFilterMetricProcessorTelemetry

=== FAIL: . TestFilterMetricProcessorTelemetry (re-run 1) (0.00s)
    generated_component_telemetry_test.go:55: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/processor/filterprocessor/generated_component_telemetry_test.go:55
        	            				/tmp/opentelemetry-collector-contrib/processor/filterprocessor/metrics_test.go:423
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 3
        	Test:       	TestFilterMetricProcessorTelemetry

The test is likely a failure of the new metrics being emitted and the original test not expecting them to be there. Please open a PR to update the test as a follow up

djaglowski · 2024-09-09T13:28:19Z

Thanks for the review @codeboten. I created open-telemetry/opentelemetry-collector-contrib#35073 to update the contrib test.

codeboten · 2024-09-09T15:18:53Z

processor/processorhelper/metrics_test.go

+	dps.AppendEmpty()
+	dps.AppendEmpty()
+
+	metricReader := sdkmetric.NewManualReader()


Could the test code here use the generated test code? Can be done in a follow up PR

…35073) This updates collector modules and fixes a test that was broken by open-telemetry/opentelemetry-collector#10910 --------- Signed-off-by: Dan Jaglowski <jaglows3@gmail.com> Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com> Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>

) #### Description Implements ingoing and outgoing counts as described in open-telemetry#10708.

@djaglowski

Follow up for #10910 (comment), ping @djaglowski as the author of the PR Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

…pen-telemetry#35073) This updates collector modules and fixes a test that was broken by open-telemetry/opentelemetry-collector#10910 --------- Signed-off-by: Dan Jaglowski <jaglows3@gmail.com> Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com> Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>

…s Undefined Behavior (#11349) The main issue is that after #10910 the err variable is shared between requests because it uses the same address as the err defined outside the func. This is an UB because we are overwriting memory and will cause crashes like #11335, where the check for not nil happens then gets overwrite with nil and crashes. Fixes #11350 --------- Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

…pen-telemetry#35073) This updates collector modules and fixes a test that was broken by open-telemetry/opentelemetry-collector#10910 --------- Signed-off-by: Dan Jaglowski <jaglows3@gmail.com> Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com> Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>

…s Undefined Behavior (open-telemetry#11349) The main issue is that after open-telemetry#10910 the err variable is shared between requests because it uses the same address as the err defined outside the func. This is an UB because we are overwriting memory and will cause crashes like open-telemetry#11335, where the check for not nil happens then gets overwrite with nil and crashes. Fixes open-telemetry#11350 --------- Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

djaglowski force-pushed the processor-helper-metrics branch from 259cf00 to f574d98 Compare August 19, 2024 19:37

djaglowski marked this pull request as ready for review August 19, 2024 22:20

djaglowski requested review from a team and TylerHelmuth August 19, 2024 22:20

iblancasa approved these changes Aug 22, 2024

View reviewed changes

codeboten reviewed Sep 6, 2024

View reviewed changes

Add ingoing and outgoing counts to processorhelper

4421a21

djaglowski force-pushed the processor-helper-metrics branch from f574d98 to 4421a21 Compare September 6, 2024 18:58

djaglowski commented Sep 6, 2024

View reviewed changes

Fix comments

96867af

djaglowski mentioned this pull request Sep 9, 2024

[chore] Update test expectation to include processorhelper metrics open-telemetry/opentelemetry-collector-contrib#35073

Merged

codeboten approved these changes Sep 9, 2024

View reviewed changes

codeboten merged commit fa34718 into open-telemetry:main Sep 9, 2024
47 of 49 checks passed

github-actions bot added this to the next release milestone Sep 9, 2024

codeboten mentioned this pull request Sep 9, 2024

[chore] cleaning up test to use mdatagen test harness #11098

Merged

dmathieu pushed a commit to dmathieu/opentelemetry-collector that referenced this pull request Sep 10, 2024

Add ingoing and outgoing counts to processorhelper (open-telemetry#10910

5480910

) #### Description Implements ingoing and outgoing counts as described in open-telemetry#10708.

codeboten mentioned this pull request Sep 10, 2024

[processorhelper] deprecating unused methods #11083

Merged

djaglowski deleted the processor-helper-metrics branch September 10, 2024 15:55

bogdandrutu pushed a commit that referenced this pull request Sep 10, 2024

[chore] cleaning up test to use mdatagen test harness (#11098)

340e775

Follow up for #10910 (comment), ping @djaglowski as the author of the PR Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

djaglowski mentioned this pull request Sep 11, 2024

[processorhelper] Add throughput metrics #11147

Closed

djaglowski mentioned this pull request Sep 30, 2024

WIP - Demonstrate graph-based metrics #11311

Closed

bogdandrutu mentioned this pull request Oct 4, 2024

Fix data race condition, concurrent writes to the err variable, causes Undefined Behavior #11349

Merged

github-actions bot mentioned this pull request Nov 11, 2024

Link Checker Report signalfx/splunk-otel-collector#5593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ingoing and outgoing counts to processorhelper #10910

Add ingoing and outgoing counts to processorhelper #10910

djaglowski commented Aug 19, 2024

codecov bot commented Aug 19, 2024 •

edited

Loading

djaglowski commented Sep 3, 2024

codeboten left a comment

codeboten Sep 6, 2024

djaglowski Sep 6, 2024

codeboten Sep 6, 2024

djaglowski Sep 6, 2024

djaglowski left a comment

djaglowski Sep 6, 2024

djaglowski commented Sep 6, 2024

codeboten commented Sep 6, 2024

djaglowski commented Sep 9, 2024

codeboten Sep 9, 2024

Add ingoing and outgoing counts to processorhelper #10910

Add ingoing and outgoing counts to processorhelper #10910

Conversation

djaglowski commented Aug 19, 2024

Description

codecov bot commented Aug 19, 2024 • edited Loading

Codecov Report

djaglowski commented Sep 3, 2024

codeboten left a comment

Choose a reason for hiding this comment

codeboten Sep 6, 2024

Choose a reason for hiding this comment

djaglowski Sep 6, 2024

Choose a reason for hiding this comment

codeboten Sep 6, 2024

Choose a reason for hiding this comment

djaglowski Sep 6, 2024

Choose a reason for hiding this comment

djaglowski left a comment

Choose a reason for hiding this comment

djaglowski Sep 6, 2024

Choose a reason for hiding this comment

djaglowski commented Sep 6, 2024

codeboten commented Sep 6, 2024

djaglowski commented Sep 9, 2024

codeboten Sep 9, 2024

Choose a reason for hiding this comment

codecov bot commented Aug 19, 2024 •

edited

Loading