2649 - Change receiver obsreport helpers pattern to match the Processor/Exporter by humivo · Pull Request #3227 · open-telemetry/opentelemetry-collector

humivo · 2021-05-18T21:27:47Z

Description: Change obsreport helpers for receiver to use the same pattern with structs and helpers as Processor/Exporter.

Link to tracking Issue: Issue #2649

receiver/scraperhelper/scrapercontroller.go

receiver/zipkinreceiver/trace_receiver.go

receiver/prometheusreceiver/internal/transaction.go

receiver/otlpreceiver/logs/otlp.go

bogdandrutu · 2021-05-19T16:40:15Z

receiver/kafkareceiver/kafka_receiver.go


 		ctx := obsreport.ReceiverContext(session.Context(), c.id, transport)
-		ctx = obsreport.StartTraceDataReceiveOp(ctx, c.id, transport)
+		rec := obsreport.NewReceiver(obsreport.ReceiverSettings{ReceiverID: c.id, Transport: transport})


Same here, you can cache to object. Please apply the caching to all the non-test code where you can.

Ok I applied to caching to all non-test code in my latest commit

receiver/jaegerreceiver/trace_receiver.go

bogdandrutu · 2021-05-19T17:29:43Z

receiver/zipkinreceiver/trace_receiver.go

 	transportTag := transportType(r, asZipkinv1)
 	ctx = obsreport.ReceiverContext(ctx, zr.id, transportTag)
-	ctx = obsreport.StartTraceDataReceiveOp(ctx, zr.id, transportTag)
+	zr.receiver = obsreport.NewReceiver(obsreport.ReceiverSettings{ReceiverID: zr.id, Transport: transportTag})


Here you cannot "cache" since you don't know the transport (actually it will cause a race condition). you cannot do things blindly :) Please check everywhere if that is the case.

Ohhh ok I thought so. I just fixed this in the latest commit. I think that was the only place where we do not know the transport.

bogdandrutu

Same comments apply everywhere

bogdandrutu · 2021-05-19T18:29:54Z

receiver/jaegerreceiver/trace_receiver.go

 func (h *agentHandler) EmitBatch(ctx context.Context, batch *jaeger.Batch) error {
 	ctx = obsreport.ReceiverContext(ctx, h.id, h.transport)
-	ctx = obsreport.StartTraceDataReceiveOp(ctx, h.id, h.transport)
+	h.obsrecv = obsreport.NewReceiver(obsreport.ReceiverSettings{ReceiverID: h.id, Transport: h.transport})


What about here? Does seem that you initialize the obsrecv every request so you will have a race. Probably initialize in the ctor if transport is known.

Now initialized in the ctor in latest commit

bogdandrutu · 2021-05-19T18:30:44Z

receiver/jaegerreceiver/trace_receiver.go


 	ctx = obsreport.ReceiverContext(ctx, jr.id, grpcTransport)
-	ctx = obsreport.StartTraceDataReceiveOp(ctx, jr.id, grpcTransport)
+	jr.obsrecv = obsreport.NewReceiver(obsreport.ReceiverSettings{ReceiverID: jr.id, Transport: grpcTransport})


Same here. Or have different instances per transport is better.

Initialized in ctor with different instances per transport

bogdandrutu · 2021-05-19T18:31:06Z

receiver/jaegerreceiver/trace_receiver.go


 	ctx = obsreport.ReceiverContext(ctx, jr.id, collectorHTTPTransport)
-	ctx = obsreport.StartTraceDataReceiveOp(ctx, jr.id, collectorHTTPTransport)
+	jr.obsrecv = obsreport.NewReceiver(obsreport.ReceiverSettings{ReceiverID: jr.id, Transport: collectorHTTPTransport})


Same here you modify the same variable.

Now initialized in the ctor in latest commit

bogdandrutu · 2021-05-19T18:32:00Z

receiver/kafkareceiver/kafka_receiver.go


 		ctx := obsreport.ReceiverContext(session.Context(), c.id, transport)
-		ctx = obsreport.StartTraceDataReceiveOp(ctx, c.id, transport)
+		c.obsrecv = obsreport.NewReceiver(obsreport.ReceiverSettings{ReceiverID: c.id, Transport: transport})


You re-initialize the variable every request, this is not caching, you need to initialize it in the ctor of the tracesConsumerGroupHandler

Now initialized in the ctor in latest commit

bogdandrutu · 2021-05-19T21:56:30Z

@humivo are you ready to also upgrade contrib right away? Otherwise better to keep as deprecated the old funcs and implement them as NewReceiver + calls into the new funcs, then we can upgrade contrib and change receivers one by one.

humivo · 2021-05-19T22:03:41Z

@humivo are you ready to also upgrade contrib right away? Otherwise better to keep as deprecated the old funcs and implement them as NewReceiver + calls into the new funcs, then we can upgrade contrib and change receivers one by one.

Yes I will be ready to upgrade the contrib right away. But I am open to whichever option is preferred.

bogdandrutu · 2021-05-19T22:08:09Z

Let's do the deprecation one, so I can ensure myself that I will not be the one doing it :)))

humivo · 2021-05-19T22:16:24Z

Let's do the deprecation one, so I can ensure myself that I will not be the one doing it :)))

Ok I'll update the PR right now with the deprecated old funcs

bogdandrutu · 2021-05-19T23:37:33Z

@humivo waiting for that :)

humivo · 2021-05-20T15:59:53Z

@humivo waiting for that :)

Yup sorry for the delay! Just made the fix.

…or/Exporter (open-telemetry#3227) * Change Receiver to match pattern with structs and helpers * Added support for ReceiverSettings * Add receiver to end functions * Add comments for new receiver * Add entry to changelog * Cache the recevier in structs * Cache the receiver in other files * Fix name for var and fix caching * Fix caching so there is no race * Fix unit tests * Add deprecated old funcs back * Fix comments

humivo added 4 commits May 17, 2021 14:47

Change Receiver to match pattern with structs and helpers

eeb679d

Added support for ReceiverSettings

aaeb01f

Add receiver to end functions

40d5e4c

Add comments for new receiver

2285662

humivo requested a review from a team May 18, 2021 21:27

humivo added 2 commits May 18, 2021 17:31

Merge branch 'open-telemetry:main' into 2649-ChangeReceiverPattern

c1563bf

Add entry to changelog

fe366e5

bogdandrutu reviewed May 18, 2021

View reviewed changes

Cache the recevier in structs

1da990a

bogdandrutu reviewed May 19, 2021

View reviewed changes

Cache the receiver in other files

790c029

bogdandrutu reviewed May 19, 2021

View reviewed changes

Fix name for var and fix caching

2200283

bogdandrutu requested changes May 19, 2021

View reviewed changes

humivo added 2 commits May 19, 2021 13:19

Fix caching so there is no race

fc76598

Fix unit tests

c66749a

humivo requested a review from bogdandrutu May 19, 2021 21:22

pavankrish123 mentioned this pull request May 19, 2021

TestBallastMemory - Intermittent test failure #3233

Closed

bogdandrutu approved these changes May 19, 2021

View reviewed changes

humivo added 2 commits May 20, 2021 08:28

Add deprecated old funcs back

23faad5

Fix comments

d37e24c

bogdandrutu merged commit f7674b2 into open-telemetry:main May 20, 2021

humivo mentioned this pull request May 20, 2021

Change obsreport helpers for receiver to use the new pattern created in Collector open-telemetry/opentelemetry-collector-contrib#3436

Closed

Conversation

humivo commented May 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdandrutu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdandrutu commented May 19, 2021

Uh oh!

humivo commented May 19, 2021

Uh oh!

bogdandrutu commented May 19, 2021

Uh oh!

humivo commented May 19, 2021

Uh oh!

bogdandrutu commented May 19, 2021

Uh oh!

humivo commented May 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants