New Relic integration uses deprecated code

[nc-goulde]

Describe the bug
The code here https://github.com/launchdarkly/ruby-server-sdk/blob/master/lib/ldclient-rb/newrelic.rb checks for the existence of the add_custom_parameters method this method has been removed for about 3 years https://github.com/newrelic/rpm/blob/master/CHANGELOG.md#v400 so this method will always return false for NR_ENABLED.

To reproduce
Run the ruby-server-sdk with rpm version > 4
check the state of NR_ENABLED in the client.

Expected behavior
NR_ENABLED is true when rpm is installed.

SDK version
5.6.2

Language version, developer tools
ruby 2.5.3
newrelic_rpm 6

OS/platform
OS X Catalina

[eli-darkly]

I think that there may be a deeper issue: namely, even if NR_ENABLED is true, the NewRelic integration is not actually doing anything.

This feature was implemented very early in the history of the Ruby SDK, as part of a method that was almost immediately deprecated, and when the deprecated method's implementation was changed to call the newer version of the method, I believe the NewRelic functionality was lost; the annotate_transaction method in the file you're looking at is no longer being called under any circumstances. In effect, this feature was never fully implemented, this is dead code, and you are the first to have noticed that it even existed (or at least the first to have mentioned it).

So, thanks for bringing it to our attention. We will need to revisit this code and, assuming this is a feature people would find useful, reimplement it and make sure we have a testing strategy in place to ensure that it works and remains working.

[eli-darkly]

[Tracked internally as 70253]

[nc-wittj]

Thanks for responding! We've decided it would be more valuable to create our own integration with NewRelic, since just attaching an attribute doesn't have enough metrics for us. I think we're considering either NewRelic transaction segments (since LD is an external service) or New Relic events (or maybe both?). One concern with merely attaching the flag as an attribute is if the feature flag is called multiple times in the same transaction with different user contexts. In that case only the last write wins, and information is lost. At this point I think I'm leaning more towards NewRelic events.

At any rate, I'm not sure if this is something that should be in the purview of the LD SDK. And if it is, there should at least be a way to disable it (if you roll your own, you don't want the LD SDK overwriting it, or adding duplicate info).

If you decide to remove this, you may want to update your docs, since that's why we thought this feature might exist. It also seems to be implemented in the Java SDK, and that one also seems to be calling the same method (so it may not work either).

[eli-darkly]

Thanks, we had missed that mention on the docs page and we will remove the reference to Ruby for now.

I'm not sure what you mean about the Java SDK implementation. The method that it looks for using reflection is com.newrelic.api.agent.NewRelic.addCustomParameter, which as far as I can tell still exists in the NewRelic API.

[nc-wittj]

It looks like you are correct that the NewRelic Java SDK still refers to attributes as "parameters". I assumed NewRelic would've changed the wording for all their SDKs. Thanks for updating the docs!