Avoid race conditions when handling data

[florianl]

Initialize local variable only once and make sure expected APMData is handled first.

[lahsivjar]

I am not sure about this, no data is not an error for the batch and I don't like returning no data errors. Now that we have already handled no data case in ForwardAPMData, can we remove this?

[florianl]

What is your suggestion to return for thelen(apmData.Data) == 0 case in AddAgentData()?
Just because there is a check in ForwardAPMData() does not prevent other users of AddAgentData() running into this issue.

[lahsivjar]

My point is that for the batch having an empty data is not an error. The batch can just not add data in this case. If we want to protect against edge cases we should do it at the source (like how we do in ForwardAPMData).

[lahsivjar]

Also, if we want to be more specific in handling the behaviour, we could introduce another method in the batch to check the metadata status, something like bool HasMetadata() and incorporate it where we require such a check.

[florianl]

I still consider the use case len(apmData.Data) == 0 in AddAgentData() dangerous even if the only caller atm ™️ is using a check. What makes this package and its API safe to use, if something else calls AddAgentData()?
Even if no data is considered a non error case, what should it be considered instead and how should it be treated? With the current behavior it seems to run into issues.

we could introduce another method in the batch to check the metadata status, something like bool HasMetadata() and incorporate it where we require such a check.

If AddAgentData() implements such a check, what should be returned in a case where

• there is not metadata but len(apmData.Data) != 0. Is in this case data loss the expected result?
• there is metadata but len(apmData.Data) == 0

[lahsivjar]

What makes this package and its API safe to use, if something else calls AddAgentData()?

Agent data here refers to data from the APM agents and intake-v2 is the only supported protocol. What other cases do you have in mind here?

there is not metadata but len(apmData.Data) != 0. Is in this case data loss the expected result?

If data from the agent is not nil then it MUST have the metadata as the first line.

there is metadata but len(apmData.Data) == 0

apmData.Data is including the metadata so this is also not possible. We could have only metadata in the body and that is okay.

[christos68k]

@lahsivjar So essentially in your view, ErrNoData and associated logic is redundant as "no data" can take place during normal operation, and if we want to check for this case, we should do it before calling AddAgentData. Is that correct?

[lahsivjar]

Basically, yes! I don't have a strong opinion here so feel free to ignore it.

[christos68k]

I think the fix that @florianl implements in this PR (not exiting the processing loop when len(data.Data) == 0) is important and should be merged. @lahsivjar Do we need anything else to get this merged?

[christos68k]

@lahsivjar So essentially in your view, ErrNoData and associated logic is redundant as "no data" can take place during normal operation, and if we want to check for this case, we should do it before calling AddAgentData. Is that correct?

[lahsivjar]

Do we need anything else to get this merged?

I think with sync.Once to nil check we should be good to merge. The remaining point on the ErrNoData is not that important and we can ignore it if it doesn't make sense to others.