Update enrichment to handle existing attributes

[isaacaflores2]

Overview

Traditional APM event ingestion utilized enrichment from the apm-data lib. With the introduction of the receiver/elasticapmintake component (which is using apm-data internally) it is possible for a collector to have a pipeline such as: receiver/elasticapmintake -> processor/elasticapm.

The processor/elasticapm and the opentelemetry-lib should account for this by not overriding any pre-existing elastic attributes. This will preserve the existing values and avoid any unintended changes to elastic attributes

This is a follow up to #237

Changes

• Added attribute.IsEmpty helper function which checks if an attribute exists or is empty. I opted to use this approach since in some it allows the logic to exit early when the attribute exists. The downside is that it adds a lot more if statements everywhere.
  • The alternative would be to add a wrapper around Attributes().PutStr but this would require the enrichment logic to always attempt to derive the new value which would be a little less efficient.
• Updated all enrichment logic to only set an attribute if it does not exists or is empty
  • The logic around transaction.type required special consideration since the library itself can set the transaction.type
• Updated isElasticTransaction to check processor.event. This avoids having to derive if a span is an elastic transaction when it is already explicitly considered a transactions. It also avoids reclassifying a span to a transaction or a transaction to a span.

[lahsivjar]

The previous logic checked for existence of the attribute key, under what cases will we have a key without any values set?

[isaacaflores2]

The only case where this can happen is if an empty string is explicitly for the processor.event. I added attribute.IsEmpty to have a consistent way to check if an attr exists/is empty but I think it also helps cover this edge case where an empty attr value is set.

[simitt]

I'm a bit worried about introducing inconsistent behaviour for zero values - e.g. why is a zero value for string handled differently than a zero value for int? Explicitly setting a string value to empty would be treated differently than explicitly setting an int value to 0.

Is it necessary to check for IsEmpty rather than just for exists?

The only case where this can happen is if an empty string is explicitly for the processor.event.

Where would this be a valid usecase where the processor.event would then later be overwritten?

[simitt]

The alternative would be to add a wrapper around Attributes().PutStr but this would require the enrichment logic to always attempt to derive the new value which would be a little less efficient.

Can you expand on this? When calling Attributes().PutStr one already provides the new value, and before calling your changes now always check the existing value. What exactly would be less efficient with the wrapper?
If there is no considerable overhead with the wrapper, I'd prefer it as it makes reading the code much easier, and it makes it harder to forget about the exists check when adding new logic.

[simitt]

I'm a bit worried about introducing inconsistent behaviour for zero values - e.g. why is a zero value for string handled differently than a zero value for int? Explicitly setting a string value to empty would be treated differently than explicitly setting an int value to 0.

Is it necessary to check for IsEmpty rather than just for exists?

The only case where this can happen is if an empty string is explicitly for the processor.event.

Where would this be a valid usecase where the processor.event would then later be overwritten?

[simitt]

Why is this removed now and an existing s.serviceInstanceID overwritten? If this is an intended change for apm logic compliance, it might need to go into a dedicated PR to not mix with the existing attribute handling.