[BUG] FillApmServerInfo : Invalid ElasticApm_ApiKey throws object not set exception

[stephdep]

APM Agent version

Latest Main branch code on 16/6/2022

Environment

Operating system and version:

.NET Framework/Core name and version (e.g. .NET 4.6.1, NET Core 3.1.100) :
.Net 6

Application Target Framework(s) (e.g. net461, netcoreapp3.1):
.Net 6

Describe the bug

When connecting using an ElasticApm_ApiKey that is invalid, an exception is thrown when calling app.UseAllElasticApm(Configuration);

reason is that the Elastic.Apm.ServerInfo.ApmServerInfoProvider FillApmServerInfo does a request that succeeds, but does not return any data, and therefore the deSerialisation returns metadata null, then doing metadata["version"] throws an "object not set to an instance of an object" exception.

				var metadata = serializer.Deserialize<JObject>(jsonReader);
				**var version = metadata["version"];**

To Reproduce

Steps to reproduce the behavior:
Configure an invalid Api Key in ElasticApm_ApiKey
use the app.UseAllElasticApm(Configuration); as explained for .Net core.
an exception is thrown

Expected behavior

improve the code :

• include the Deserialize in the foreseen try catch
• what if strVersion is null, no callbackInFinish is called

Actual behavior

throws Object not set to an instance of an object exception

[gregkalapos]

Thanks for reporting! We'll look at this - clearly something we'll need to fix.

[gregkalapos]

To be clear, this whole thing was in a try-catch block.

I agree we can have a better parsing logic - that's what @z1c0 fixes in #1787.

But by looking at the current code (before #1787): what happened is that there was indeed an exception which was handled in the exact same method and this ended up being a warning log. So that exception never bubbled up into the app's code.

#1787 will make this even better with better logging.

[z1c0]

But by looking at the current code (before #1787): what happened is that there was indeed an exception which was handled in the exact same method and this ended up being a warning log. So that exception never bubbled up into the app's code.

That's right - the exception did not bubble up to the application. However, this led to a NRE showing up in the agent log.
This should be fixed in #1787 and - as you said - also better logging will be in place.