AddCustomAttribute() fails to update existing attributes when attribute limit reached.

[vuqtran88]

Description
AddCustomAttribute() fails to update existing attributes with new value when attribute limit reached (65 attribute per transaction). This is due to this line of code:

newrelic-dotnet-agent/src/Agent/NewRelic/Agent/Core/Attributes/AttributeValueCollection.cs

Line 177 in 2ba99ea

if (!ValidateCollectionLimits(attribDef.Classification, attribDef.Name))

When attribute limit reached, the agent doesn't do anything.

Expected Behavior
AddCustomAttribute() should update existing attributes even when the limit reached.

[workato-integration]

https://issues.newrelic.com/browse/NEWRELIC-3604

[workato-integration]

Commented by chynes:
This is another ticket that seemed straightforward until I started looking into it.

There are two implementations of Attributes: one used by Spans, another by Transactions. The code used by Spans works as expected, except for the bug described here. If you're at the max number of attributes, you can no longer update existing ones.

The one used by Transactions, however, is completely different. It's implemented not as a Dictionary but as a List. That means if you set the same Attribute multiple times, each one counts against the total, which is not correct. The duplicates are removed at harvest time, but we may have already discarded a number of valid Attributes by then.

I propose we move the Transaction code to using a Dictionary before fixing the issue described in this ticket, as some of the code is shared. However, this is how the original implementation worked, and was converted to a List as part of a performance optimization milestone. We will need to be careful not to make performance worse. I'm confident we can accomplish this, but we have run out of time for this milestone. I have a local branch that I started work on that I can pick up again when we revisit this ticket.

[workato-integration]

Jira CommentId: 124225
Commented by angelatan:

[~jcoleman]is currently troubleshooting this ticket, and here is a quick update for folks who are following the ticket:

• The storage mechanism for custom attributes has been updated to support updating custom attributes that already exist in a transaction, even when the attribute limit is reached.
• Updating test coverage to handle edge cases and unifying the attribute storage mechanism used for transactions and spans.

[workato-integration]

Jira CommentId: 128280
Commented by jcoleman:

WIP changes can be found on the branch:
jc/attribute-limit-fix

 

The Transaction attribute implementation has been converted to use a similar implementation as the Span attribute collection, but over 150 unit tests are failing. It appears that there are some undocumented expectations about behaviors of this class.

[workato-integration]

Jira CommentId: 128298
Commented by chynes:

[~jcoleman] - Yeah, a number of the unit tests specifically validate the "insert all then sort and deduplicate" behavior. It was one of the reasons I ran out of time on my investigation.

[workato-integration]

Jira CommentId: 129931
Commented by jcoleman:

Status update: 
A draft PR is open that resolves the issue, and adds test coverage for the scenarios identified in the ticket. 

These change still need to be performance tested. There are some concerns about increased locking with the new changes that may need to be optimized depending on findings.

[workato-integration]

Jira CommentId: 129931
Commented by jcoleman:

Status update: 
A [draft PR|https://github.com//pull/1335] is open that resolves the issue, and adds test coverage for the scenarios identified in the ticket. 

These change still need to be performance tested. There are some concerns about increased locking with the new changes that may need to be optimized depending on findings.

[workato-integration]

Work has been completed on this issue.