newrelic / newrelic-telemetry-sdk-java

Java library for sending telemetry data to New Relic
Apache License 2.0
41 stars 37 forks source link

Replace UUID.randomUUID() with a faster implementation #292

Closed tmancill closed 2 years ago

tmancill commented 2 years ago

Instantiating a TelemetryBatch calls UUID.randomUUID(), which in turn uses SecureRandom to generate 16 random bytes. When generating TelemetryBatch at high rates, this can exhaust the entropy source and is computationally expensive.

This PR replaces all calls to UUID.randomUUID() with a utility method that generates UUIDs using ThreadLocalRandom.current().nextLong().

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

Steven-M-Brown commented 2 years ago

Going to necro this PR.

While I understand (and actually agree with) the desire to update this the way this is done has a subtle bug in that you are generating a V4 random UUID but ignoring the reserved bits in the standard specifying the version/variant. In most cases this won't be an issue but if some system tries to do something with that UUID it may cause issues.

Should be an easy fix, just clear/set the appropriate bits after generating the values.

tmancill commented 2 years ago

Hi @Steven-M-Brown . Thanks for reviewing and bringing this up. It's a little strange that the UUID() constructor doesn't handle adherence - that is that UUID.randomUUID() is the only code that generates a V4 random UUID, but that's an issue for the JDK. I will put up a PR to implement the version logic from randomUUID().

Just curious - did you run into this because a downstream system is expecting V4 UUIDs?

For reference: https://www.ietf.org/rfc/rfc4122.txt

Steven-M-Brown commented 2 years ago

I would imagine the JDK doesn't because they don't want to cover every case and that constructor could conceivable be used to implement any unsupported variant of it whereas randomUUID() does because that is pretty much the current definition of a type 4 UUID as far as I know.

I just happened to notice because I was looking through the change logs before validating the new agent version in our infrastructure.