NewRelicRegistry sometimes skip publishing metrics on close

[shirolimit]

According to MeterRegistry contract, close call on it should release all registry resources and send all unpublished meters.

NewRelicRegistry doesn't override close() method and default implementation simply calls publish(). The problem is that publish() method uses TelemetryClient that uses executor to to a scheduled send. As a result, if close() is called on application shutdown metrics aren't actually sent.

A possible solution would be to override close() method and to publish metrics synchronously in it.

[shirolimit]

Oh, one more option is to simply call TelemetryClient.shutdown() on NewRelicRegistry.close().

[shirolimit]

It seems that it has already been fixed in https://github.com/newrelic/micrometer-registry-newrelic/commit/5e42c309a238456ba3759157030d2f341e455f83 but it is not present in 0.5.0 release.

[breedx-nr]

Hi @shirolimit .

The commit you referenced above was part of #82, which was merged on Jun 17th. Version 0.5.0 seems to have been released in September...so what makes you believe that that change isn't present?

I've also confirmed that that change is still in place on main branch: https://github.com/newrelic/micrometer-registry-newrelic/blob/main/src/main/java/com/newrelic/telemetry/micrometer/NewRelicRegistry.java#L156

[shirolimit]

Hi @breedx-nr .

Two things:

• I checked out v0.5.0 tag and checked that this piece of code is not there
• I checked commit log, here it is:

  2f9234c (HEAD -> main, origin/main, origin/HEAD) Merge pull request #104 from ebullient/rename
  543e28f Use consistent package space for newrelic registry
  cdb7837 Merge pull request #100 from newrelic/update_docs_for_micrometer_version
  3655135 newer version of micrometer core works great.
  80320ea Merge pull request #98 from newrelic/upgrade_dep_versions
  8a73f8c upgrade sdk and micrometer versions
  bf29eac Merge pull request #95 from newrelic/no-more-azure
  d20edcf Configure publishing tasks
  672214f Add snapshot in gradle
  c51a4f8 Javadoc fixes
  8e5f130 Add github workflows
  bb3fb76 Remove Azure Pipeline
  a9563e0 Merge pull request #92 from newrelic/update-gradle
  e439d11 disable gradle metadata
  4345f07 Merge pull request #90 from newrelic/update-gradle
  14f4085 Merge branch 'main' into update-gradle
  c1a3bdf update gradle
  5ad4ffe master -> main
  a68435b Merge pull request #89 from newrelic/remove_insights
  2719f8c just Insert API Key
  7457c3d Merge pull request #85 from newrelic/update_sdk_version
  e5144d3 api -> implementation
  7bc0b61 update to sdk version 0.6.5 and change construction.
  0d9a6be Merge pull request #82 from paypay/master
  0bc3104 Merge branch 'master' into master
  d2cef9d Merge pull request #83 from newrelic/update-mock-server
  dde7a17 Update mock-server
  ef6f14a Merge pull request #1 from paypay/issue/80-shutdonw-telemetryclient-in-close
  e8f6765 add comments to clarify why this order is needed
  5e42c30 shutdown telemetryclient in close
  0f90447 Merge pull request #79 from newrelic/javadoc.io-links
  248935a make the javadoc link more clear
  7fb4605 Merge pull request #78 from newrelic/javadoc.io-links
  2b242f4 Add javadoc.io links
  6fd5791 (tag: v0.5.0, origin/0.5.0_release_branch) Merge pull request #75 from newrelic/jasonjkeller/add-release-details

As you can see ef6f14a Merge pull request #1 from paypay/issue/80-shutdonw-telemetryclient-in-close is not included in v0.5.0. For some reason the latest release was based on April's commit.

[breedx-nr]

Ooh, sure enough! You're right!

It looks like the tag AND release build were generated back in April, as you discovered...but the github release was backfilled to show it in September. I will look at cutting a new release build. Sorry for the confusion!

[breedx-nr]

@shirolimit We released v0.6.0 today that should contain these changes. Please give it a shot and let us know if this helps with buffer flushing on close. Feel free to reopen if there are additional concerns, and thanks for bringing it to our attention.