Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polish "Add metrics support for Netty 4.x" #3768

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Apr 13, 2023

This PR polishes the "Add metrics support for Netty 4.x" changes a bit.

See gh-3742

@jonatan-ivanov jonatan-ivanov added polish A general improvement (naming things, fixing minor issues, etc.) module: micrometer-binders labels Apr 13, 2023
@jonatan-ivanov
Copy link
Member

@izeye Thank you for the changes! Could you please fix the issue number in the commit message and in the description? I think it is not the one that this PR polishes.

@jonatan-ivanov jonatan-ivanov added this to the 1.11.0 milestone Apr 13, 2023
@izeye
Copy link
Contributor Author

izeye commented Apr 13, 2023

@jonatan-ivanov Oops, sorry. I fixed them now.

@jonatan-ivanov jonatan-ivanov merged commit 23804d5 into micrometer-metrics:main Apr 13, 2023
@izeye izeye deleted the gh-3742 branch April 13, 2023 22:42
@franz1981
Copy link

franz1981 commented Apr 24, 2023

@jonatan-ivanov what about the allocatorId ? I didn't yet opened an issue but would be better to:

  • use a CopyOnWriteArraySet with the refs of allocators already registered to verify a certain allocator isn't observed yet
  • and a static atomic long to create runtime fake identity for the allocator just registered

@bclozel
Copy link
Contributor

bclozel commented Apr 24, 2023

@franz1981 the current design assumes that there are various ways to instrument Netty (and the allocator) here:

  • manually during startup, when resources like allocators are manually set and known. In this case, there would be no need to maintain the list of already registered allocators
  • at runtime, lazily, when an allocator is seen during channel init. In this case, it's a good idea to maintain such a list and prevent duplicate instrumentations

We can't choose this for libraries, this is why we chose to remain neutral in Micrometer's instrumentation. Does this make sense?

@franz1981
Copy link

franz1981 commented Apr 24, 2023

We can't choose this for libraries, this is why we chose to remain neutral in Micrometer's instrumentation. Does this make sense?

Yeah, that's fair, but hashCode suffer from collision, hence, although neutral is not a proper identity and can cause 2 different allocator refs to be reported as the same - it's rare, but possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: micrometer-binders polish A general improvement (naming things, fixing minor issues, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants