[connector/exceptions] Add support for exemplars in exceptionsconnector #31819

marctc · 2024-03-18T18:44:08Z

Description:
Adds support for exemplars fort the generated metrics from exceptions.
It relates the span and trace id with metrics.

The ingestion needs to be enabled via config (disbled by default).

Link to tracking Issue: Resolves #24409

Documentation: Added documentation for enabling generation of exemplars.

github-actions · 2024-04-03T05:19:54Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2024-04-18T05:19:51Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

jpkrohling · 2024-04-30T10:21:20Z

connector/exceptionsconnector/config.go

@@ -15,6 +15,10 @@ type Dimension struct {
 	Default *string `mapstructure:"default"`
 }

+type ExemplarsConfig struct {


I think you might not need the suffix Config here?

connector/exceptionsconnector/config.go

codecov · 2024-04-30T11:36:04Z

Codecov Report

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

Project coverage is 83.11%. Comparing base (cbf003e) to head (77e9bf1).
Report is 481 commits behind head on main.

Files	Patch %	Lines
connector/exceptionsconnector/connector_metrics.go	80.64%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #31819      +/-   ##
==========================================
+ Coverage   81.88%   83.11%   +1.23%     
==========================================
  Files        1858     1977     +119     
  Lines      172718   189370   +16652     
==========================================
+ Hits       141423   157400   +15977     
- Misses      26984    27139     +155     
- Partials     4311     4831     +520

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

niftyhoot

@marctc This is a very nice enhancement. Thank you!

niftyhoot · 2024-05-04T17:21:23Z

connector/exceptionsconnector/connector_metrics.go

+			exc.exemplars.At(i).SetTimestamp(timestamp)
+		}
+		exc.exemplars.CopyTo(dp.Exemplars())
+		exc.attrs.CopyTo(dp.Attributes())


@marctc - Under heavy load conditions or in case of an application gone into some sort of crash loop, could exc.exemplars possibly have too many entries to copy?
The situation may be worse if the end users add custom dimensions that can possibly have large number of unique measurements (cardinality)
So, I wonder if it could probably overwhelm the system (or in worst case, panic) in the CopyTo operation.

I see that spans-to-metrics connector has some safeguard mechanisms like max_per_data_point (along with dimensions_cache_size and resource_metrics_cache_size)

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/connector/spanmetricsconnector/config.go#L93

Do you think for the scope of this PR is necessary or should we address it in a follow up PR?

We can do a follow up PR but I got a bit curious and tried to run an overnight stress test after patching this change in my local setup. The collectors crashed multiple times. My recommendation would be to restrict just one exemplar per data point for now.

panic: runtime error: index out of range [340043] with length 340043 goroutine 70 [running]: go.opentelemetry.io/collector/pdata/pmetric.ExemplarSlice.CopyTo({0xc02cf84f90?, 0xc02cf3e72c?}, {0xc14ea5f7d8?, 0xc05ab969b0?}) go.opentelemetry.io/collector/pdata@v1.4.0/pmetric/generated_exemplarslice.go:134 +0x12d github.com/open-telemetry/opentelemetry-collector-contrib/connector/exceptionsconnector.(*metricsConnector).collectExceptions(0xc000c62900, {0xc1363f6690?, 0xc05ab969b0?}) github.com/open-telemetry/opentelemetry-collector-contrib/connector/exceptionsconnector@v0.96.0/connector_metrics.go:161 +0x3cb github.com/open-telemetry/opentelemetry-collector-contrib/connector/exceptionsconnector.(*metricsConnector).exportMetrics(0xc000c62900, {0x3530320, 0xc001171da0}) github.com/open-telemetry/opentelemetry-collector-contrib/connector/exceptionsconnector@v0.96.0/connector_metrics.go:131 +0x25f github.com/open-telemetry/opentelemetry-collector-contrib/connector/exceptionsconnector.(*metricsConnector).ConsumeTraces(0xc000c62900, {0x3530320, 0xc001171da0}, {0xc026bbd308?, 0xc002c47498?}) github.com/open-telemetry/opentelemetry-collector-contrib/connector/exceptionsconnector@v0.96.0/connector_metrics.go:122 +0x14b go.opentelemetry.io/collector/internal/fanoutconsumer.(*tracesConsumer).ConsumeTraces(0xc001171cb0?, {0x3530320, 0xc001171da0}, {0xc026bbd308?, 0xc002c47498?})

niftyhoot · 2024-05-04T20:58:25Z

connector/exceptionsconnector/connector_metrics_test.go

+		assert.NotZero(t, exemplar.Timestamp())
+		assert.NotZero(t, exemplar.TraceID())
+		assert.NotZero(t, exemplar.SpanID())
+


Would it be a good idea to add a benchmark test ?

There's already a benchmark (BenchmarkConnectorConsumeTraces). Do you have anything in mind exactly to benchmark?

I was thinking, digging Exception events in the real world workload, sort of requires deep-packet inspection and may be we add a test that essentially find 1 in 10 traces with baggage containing exception (say, as one of the 5 span event entries) and see how fast can we generate metrics and/or log events.
But I am goo with current change. I think its functionally correct and pretty awesome work. Thank You for doing this Marc.

niftyhoot · 2024-05-06T16:02:32Z

connector/exceptionsconnector/connector_metrics.go

+	if !c.config.Exemplars.Enabled || traceID.IsEmpty() {
+		return
+	}
+	e := exc.exemplars.AppendEmpty()


Can this Append result in an unbounded growth and that in fact might be the root cause for the panic I noticed in my overnight test.
So, an alternate approach could be to reset exc.exemplars to a new slice after exc.exemplars.CopyTo(dp.Exemplars())

I think we could just add that count mechanism that spansmetricsconnector is using, wdyt?

Hello Marc - That should be good IMO. However, what do you think of resetting exc.exemplars back to pmetric.NewExemplarSlice() after doing CopyTo
I guess I do not understand the need to hang on to older exemplars post that CopyTo operation. Can you please help me understand?

You are totally right, what about now?

Looks good to me Marc. Thank you.

connector/exceptionsconnector/connector_metrics.go

…/opentelemetry-collector-contrib into exceptionsconnector/add_exemplars

niftyhoot

Thank you for doing this Marc.

connector/exceptionsconnector/config.go

jpkrohling · 2024-05-21T20:17:01Z

The tests are failing, apparently:

    connector_metrics_test.go:201: 
        	Error Trace:	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/connector/exceptionsconnector/connector_metrics_test.go:201
        	            				/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/connector/exceptionsconnector/connector_metrics_test.go:169
        	            				/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/connector/exceptionsconnector/connector_metrics_test.go:87
        	Error:      	Not equal: 
        	            	expected: 2
        	            	actual  : 1
        	Test:       	TestConnectorConsumeTraces/Test_two_consumptions

marctc · 2024-05-23T16:08:04Z

@jpkrohling now is ready to merge!

Add support for exemplars in exceptionsconnector

96250d4

marctc requested a review from jpkrohling as a code owner March 18, 2024 18:44

marctc requested a review from a team March 18, 2024 18:44

github-actions bot assigned bryan-aguilar Mar 18, 2024

github-actions bot added the connector/exceptions label Mar 18, 2024

Set Double

77e9bf1

marctc changed the title ~~Add support for exemplars in exceptionsconnector~~ [connector/exceptions] Add support for exemplars in exceptionsconnector Mar 18, 2024

github-actions bot added the Stale label Apr 3, 2024

github-actions bot closed this Apr 18, 2024

jpkrohling reopened this Apr 30, 2024

jpkrohling reviewed Apr 30, 2024

View reviewed changes

github-actions bot removed the Stale label May 1, 2024

niftyhoot reviewed May 4, 2024

View reviewed changes

marctc added 3 commits May 6, 2024 15:11

Change struct name

45d59ae

Simplify expression

d5a63dc

Merge branch 'main' into exceptionsconnector/add_exemplars

502e7fb

niftyhoot reviewed May 6, 2024

View reviewed changes

niftyhoot reviewed May 7, 2024

View reviewed changes

connector/exceptionsconnector/connector_metrics.go Show resolved Hide resolved

marctc added 3 commits May 7, 2024 16:10

Ensure capacity

4a34870

Improve coverage

f3f5374

Merge branch 'exceptionsconnector/add_exemplars' of github.com:marctc…

e6ff565

…/opentelemetry-collector-contrib into exceptionsconnector/add_exemplars

marctc requested a review from niftyhoot May 7, 2024 14:13

marctc added 4 commits May 7, 2024 16:32

Merge branch 'main' into exceptionsconnector/add_exemplars

5a80fec

Reset exemplars

826ab5d

Merge branch 'main' into exceptionsconnector/add_exemplars

13bf203

Merge branch 'main' into exceptionsconnector/add_exemplars

6256e6b

niftyhoot approved these changes May 15, 2024

View reviewed changes

jpkrohling approved these changes May 16, 2024

View reviewed changes

connector/exceptionsconnector/config.go Show resolved Hide resolved

jpkrohling added 2 commits May 16, 2024 12:41

Merge branch 'main' into exceptionsconnector/add_exemplars

231c898

Merge branch 'main' into exceptionsconnector/add_exemplars

6099031

marctc added 2 commits May 23, 2024 15:15

Merge branch 'main' into exceptionsconnector/add_exemplars

4fb4f79

Fix test

66b4d4e

Merge branch 'main' into exceptionsconnector/add_exemplars

3fcf6aa

crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label May 23, 2024

jpkrohling merged commit 4fad287 into open-telemetry:main May 24, 2024
169 checks passed

github-actions bot added this to the next release milestone May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[connector/exceptions] Add support for exemplars in exceptionsconnector #31819

[connector/exceptions] Add support for exemplars in exceptionsconnector #31819

marctc commented Mar 18, 2024 •

edited

Loading

github-actions bot commented Apr 3, 2024

github-actions bot commented Apr 18, 2024

jpkrohling Apr 30, 2024

codecov bot commented Apr 30, 2024

niftyhoot left a comment

niftyhoot May 4, 2024

marctc May 6, 2024

niftyhoot May 6, 2024

niftyhoot May 4, 2024

marctc May 6, 2024

niftyhoot May 10, 2024

niftyhoot May 6, 2024

marctc May 7, 2024

niftyhoot May 8, 2024

marctc May 10, 2024

niftyhoot May 10, 2024

niftyhoot left a comment •

edited

Loading

jpkrohling commented May 21, 2024

marctc commented May 23, 2024

[connector/exceptions] Add support for exemplars in exceptionsconnector #31819

[connector/exceptions] Add support for exemplars in exceptionsconnector #31819

Conversation

marctc commented Mar 18, 2024 • edited Loading

github-actions bot commented Apr 3, 2024

github-actions bot commented Apr 18, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 30, 2024

Codecov Report

niftyhoot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftyhoot left a comment • edited Loading

Choose a reason for hiding this comment

jpkrohling commented May 21, 2024

marctc commented May 23, 2024

marctc commented Mar 18, 2024 •

edited

Loading

niftyhoot left a comment •

edited

Loading