Do onTraceReleased asynchronously in groupbytraceprocessor #1808

pkositsyn · 2020-12-11T23:00:39Z

Description:
fixing a bug, when this processor could be overloaded.

Link to tracking Issue: #1807

Testing:
Added test for checking nonblocking onTraceReleased

Documentation:
Changed the metrics description

codecov · 2020-12-12T02:05:05Z

Codecov Report

Merging #1808 (68a2f3f) into master (f53bd40) will increase coverage by 20.05%.
The diff coverage is 73.33%.

@@             Coverage Diff             @@
##           master    #1808       +/-   ##
===========================================
+ Coverage   69.77%   89.83%   +20.05%     
===========================================
  Files          34      378      +344     
  Lines        1532    18214    +16682     
===========================================
+ Hits         1069    16362    +15293     
- Misses        391     1388      +997     
- Partials       72      464      +392

Flag	Coverage Δ
integration	`69.77% <ø> (ø)`
unit	`88.53% <73.33%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
processor/groupbytraceprocessor/event.go	`95.93% <33.33%> (ø)`
processor/groupbytraceprocessor/processor.go	`96.63% <50.00%> (ø)`
processor/groupbytraceprocessor/storage_memory.go	`88.88% <100.00%> (ø)`
receiver/prometheusexecreceiver/factory.go	`100.00% <0.00%> (ø)`
exporter/carbonexporter/exporter.go	`79.62% <0.00%> (ø)`
internal/awsxray/util.go	`100.00% <0.00%> (ø)`
...cedetectionprocessor/internal/resourcedetection.go	`97.05% <0.00%> (ø)`
receiver/memcachedreceiver/util.go	`0.00% <0.00%> (ø)`
...r/dynatraceexporter/serialization/serialization.go	`100.00% <0.00%> (ø)`
exporter/datadogexporter/metadata/metadata.go	`48.07% <0.00%> (ø)`
... and 358 more

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 f53bd40...68a2f3f. Read the comment docs.

jpkrohling

Leaving a review here, but we do need to discuss the approach first.

processor/groupbytraceprocessor/event_test.go

jpkrohling · 2020-12-14T09:41:43Z

processor/groupbytraceprocessor/event.go

+		// because last phase does not affect this processor anymore.
+		go func() {
+			if err := em.onTraceReleased(payload); err != nil {
+				em.logger.Error("onTraceReleased failed", zap.Error(err))


You'll need to include payload information to make this useful.

I don't like the idea to log payload for every error. Just imagine that the following processor starts to fail on every trace. Btw the payload is ResourceSpans, what profit do we get from logging pointers?

Ideally, this logger should be aggregate like in exporters to avoid spamming on failures

Adding the traceID would be more than enough. You can also move it to Debug instead.

Ideally, this logger should be aggregate like in exporters to avoid spamming on failures

That's the main reason to use metrics for this, so that people know that something went wrong but still don't get flooded with error messages.

People know, that something went wrong, but don't know what exactly. That's why every processor returns error - in my opinion it must be shown in logs. This is error indeed.

If it is needed, why don't you put traceID into the error? In my world error should be self-sustained

If it is needed, why don't you put traceID into the error? In my world error should be self-sustained

That's what I'm requesting :-)

I mean put it in the place where the error occured. Honestly, I don't see any profit in knowing traceID. At least in my cases with sampling I cannot do anything with ID

And currently no traceID is logged for error. Don't think recovering traceID from []ResourceSpans is the right move

pkositsyn · 2020-12-14T12:20:37Z

Also did minor changes, which improve performance. Since the processing is one-threaded, this is really crucial

github-actions · 2020-12-22T05:36:02Z

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

pkositsyn · 2020-12-24T02:48:58Z

@jpkrohling Could you take a look please?

jpkrohling

I'd prefer if this change contained only the changes that are subject to this PR. Apart from a missing test, this looks good.

processor/groupbytraceprocessor/event.go

jpkrohling · 2020-12-28T14:59:16Z

processor/groupbytraceprocessor/storage_memory.go

@@ -25,7 +25,7 @@ import (

 type memoryStorage struct {
 	sync.RWMutex
-	content                   map[string][]pdata.ResourceSpans
+	content                   map[[16]byte][]pdata.ResourceSpans


This can be pdata.TraceId, no?

You're right, but I don't like this idea, because in case of changes in the model it will implicitly affect this processor. Current approach guarantees correctness

Pretty much every processor and exporter that needs to deal with TraceID uses pdata.TraceId, @bogdandrutu usually writes scripts that updates those places en masse.

processor/groupbytraceprocessor/storage_memory.go

processor/groupbytraceprocessor/processor.go

github-actions · 2021-01-07T05:48:12Z

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

bogdandrutu · 2021-01-11T21:45:05Z

@jpkrohling review status?

jpkrohling

Once it uses the TraceID in the storage, this is ready to be merged.

github-actions · 2021-01-20T06:03:59Z

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

jpkrohling · 2021-01-20T14:52:34Z

@pkositsyn , are you interested in continuing with this one? We are very close here :-)

pkositsyn · 2021-01-20T15:54:20Z

Done

jpkrohling

Ready to be merged on green!

* Authentication processor 1/4 - Add configauth Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Authentication processor 2/4 - Add auth context and interface Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

The initialization options of the exporter are not used after the Exporter is created. Stop saving them in a field.

Do onTraceReleased asynchronously in groupbytraceprocessor

b8024a9

pkositsyn requested review from jpkrohling and a team as code owners December 11, 2020 23:00

project-bot bot added this to In progress in Collector Dec 11, 2020

github-actions bot assigned nilebox Dec 11, 2020

pkositsyn added 3 commits December 12, 2020 02:17

Add test for async release

952eda0

Fix lint

9094ca9

Fix mockprocessor to be asynchronous

065d489

jpkrohling reviewed Dec 14, 2020

View reviewed changes

pkositsyn added 4 commits December 14, 2020 14:19

do async onTraceReleased

6e827ff

remove test for unchanged logic

447416f

small optimizations

6d31102

narrow lock scope in get

5697dce

Fix lint

084c930

github-actions bot added the Stale label Dec 22, 2020

github-actions bot removed the Stale label Dec 24, 2020

jpkrohling reviewed Dec 28, 2020

View reviewed changes

Add test for async onRelease

462d72d

github-actions bot added the Stale label Jan 7, 2021

github-actions bot removed the Stale label Jan 12, 2021

jpkrohling reviewed Jan 12, 2021

View reviewed changes

github-actions bot added the Stale label Jan 20, 2021

Use pdata.TraceID as map key

68a2f3f

jpkrohling approved these changes Jan 20, 2021

View reviewed changes

Collector automation moved this from In progress to Reviewer approved Jan 20, 2021

github-actions bot removed the Stale label Jan 21, 2021

bogdandrutu merged commit ce4189c into open-telemetry:master Jan 22, 2021

Collector automation moved this from Reviewer approved to Done Jan 22, 2021

ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022

Remove options field from Jaeger exporter (#1808)

0489060

The initialization options of the exporter are not used after the Exporter is created. Stop saving them in a field.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do onTraceReleased asynchronously in groupbytraceprocessor #1808

Do onTraceReleased asynchronously in groupbytraceprocessor #1808

pkositsyn commented Dec 11, 2020 •

edited

codecov bot commented Dec 12, 2020 •

edited

jpkrohling left a comment

jpkrohling Dec 14, 2020

pkositsyn Dec 14, 2020

jpkrohling Dec 14, 2020

pkositsyn Dec 14, 2020 •

edited

jpkrohling Dec 14, 2020

pkositsyn Dec 14, 2020

pkositsyn Dec 14, 2020

pkositsyn commented Dec 14, 2020

github-actions bot commented Dec 22, 2020

pkositsyn commented Dec 24, 2020

jpkrohling left a comment

jpkrohling Dec 28, 2020

pkositsyn Dec 30, 2020

jpkrohling Jan 12, 2021

github-actions bot commented Jan 7, 2021

bogdandrutu commented Jan 11, 2021

jpkrohling left a comment

github-actions bot commented Jan 20, 2021

jpkrohling commented Jan 20, 2021

pkositsyn commented Jan 20, 2021

jpkrohling left a comment

Do onTraceReleased asynchronously in groupbytraceprocessor #1808

Do onTraceReleased asynchronously in groupbytraceprocessor #1808

Conversation

pkositsyn commented Dec 11, 2020 • edited

codecov bot commented Dec 12, 2020 • edited

Codecov Report

jpkrohling 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

pkositsyn Dec 14, 2020 • edited

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

pkositsyn commented Dec 14, 2020

github-actions bot commented Dec 22, 2020

pkositsyn commented Dec 24, 2020

jpkrohling 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

github-actions bot commented Jan 7, 2021

bogdandrutu commented Jan 11, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 20, 2021

jpkrohling commented Jan 20, 2021

pkositsyn commented Jan 20, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

pkositsyn commented Dec 11, 2020 •

edited

codecov bot commented Dec 12, 2020 •

edited

pkositsyn Dec 14, 2020 •

edited