chore: upload embedded dt response difference samples to s3

[itsmihir]

Description

Upload the embedded DT and rudder-transformation DT response difference log files to S3 instead of writing them to disk.

Linear Ticket

< PIPE-2047 >

Security

• The code changed/added as part of this pull request won't create any security issues with how the software is being used.

[codecov]

Codecov Report

Attention: Patch coverage is 47.36842% with 30 lines in your changes missing coverage. Please review.

Project coverage is 76.89%. Comparing base (fbc4abe) to head (083c853).

Files with missing lines	Patch %	Lines
...rnal/transformer/destination_transformer/logger.go	0.00%	19 Missing ⚠️
...destination_transformer/destination_transformer.go	72.22%	8 Missing and 2 partials ⚠️
processor/processor.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5792      +/-   ##
==========================================
- Coverage   76.90%   76.89%   -0.01%     
==========================================
  Files         491      491              
  Lines       67183    67206      +23     
==========================================
+ Hits        51667    51681      +14     
- Misses      12692    12703      +11     
+ Partials     2824     2822       -2     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fracasula]

Can we decrease this into a warning?

[itsmihir]

any specific reason for this? We are logging a similar entry as an error for UT mirroring here

[fracasula]

Just a suggestion, I think in hindsight we might end up having a huge amount of these and since they are not critical we might want an easy way to differentiate from the critical ones. In fact even the one I did for UT mirroring can be demoted to a warning.

[fracasula]

We shouldn't hold a mutex for long operations. Uploads can take a while and in destination_transformer.go you're just firing go routines like go CompareAndLog(...) and then hold a mutex here. This is a bad practice in my opinion we can do better. Is this mutex just to protect c.loggedEvents?

[itsmihir]

Yes, it's to make sure we don't exceed the maxLoggedEvents limit.

[itsmihir]

If we remove the mutex, are we okay with not considering maxLoggedEvents as a hard limit?

[fracasula]

Considering the use case, we should be good if are close enough to maxLoggedEvents, doesn't have to be precise at 100%, right? So an atomic would do the job quite well: #5806

What do you think of that approach?

[fracasula]

The gist of it is to find a way to avoid this scenario:

1. DT is somehow broken, we process a lot of messages and each one of them is different, ergo we end up with tons of diffs to upload
2. uploads take a very long time
3. we fire a lot of go routines continuously like go c.CompareAndLog(...) but none of them can take the lock (because of the slow upload) but we keep processing thus we keep firing go routines without control

I think the above might lead to memory issues and slow GC. At some point even in Golang there is a limit on how many go routines is too many go routines 😅

So we either use an atomic and end up logging a bit more or you come up with some other way to avoid the uncontrolled creation of go routines waiting on locks that might be locked for long periods. I'm sure there are different ways of doing this, just pointing out the potential problem but we don't have to use atomics, it was just an idea.

[ktgowtham]

Agreed to the mentioned problems. Why can't we just do a leaky logger approach?

[itsmihir]

Why can't we just do a leaky logger approach?

Are you referring to a leaky upload, or do we not even compare the payload in the first place?

[itsmihir]

@ktgowtham I tried implementing a leaky logger, but using the main context to start the leaky logger added complexity. Initially, I tried using the context passed to Transform(), but I realised that a new context is created each time.
To fix this properly, we’d need to pass the main context down to transformer.NewClients, which would require significant changes. I’m not sure if that effort is justified at this point.

[fracasula]

Approved with minor suggestion around the atomic approach. Personally I think that would be enough, without needing the leaky logger.

#5792 (comment)

[fracasula]

This is probably coming from my own branch but on a second look I think this should be updated so that we bump loggedEvents as soon as we know how many events we got. Basically something like this:

	differingResponse, sampleDiff := c.differingEvents(embeddedResponse, legacyResponse)
	if len(differingResponse) == 0 && sampleDiff == "" {
		return
	}

	c.loggedEvents.Add(int64(len(differingResponse)))

[fracasula]

The most precise I can think of is this: https://github.com/rudderlabs/rudder-server/pull/5816/files
Not overly complicated, also I don't think at this point we need a leaky logger either. The atomic approach seems safe and precise enough.