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

issue-405: quick fix for accidentally removed instrumentations #406

Conversation

mitchnull
Copy link

@mitchnull mitchnull commented Jun 1, 2022

Quick fix for accidentally removed instrumentations in ExecutorServiceMetricsBinder:

  • just return the original executorService instead of the unwrapped io.netty class

The proper fix would be to rethink the original fix for #62 and find a less brittle solution, but in the meantime this obvious error should be fixed IMO.

It would also be nice if it were backported to previous stable branches

@CLAassistant
Copy link

CLAassistant commented Jun 1, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sdelamo
Copy link
Contributor

sdelamo commented Jan 26, 2024

@jeremyg484 is this necessary?

@jeremyg484
Copy link
Contributor

@sdelamo It seems OK to me on the surface, but I haven't tested it thoroughly enough to be confident it won't break other things downstream (as Graeme mentioned in his comment on #405). I took his comment there to be an ok workaround, but maybe I'm misunderstanding.

@jeremyg484
Copy link
Contributor

I have spent some further time digging into this in relation to #679 and it's PR #702. I now have a better understanding of the intended behavior, and I think it is safe to close this PR without merging. I've confirmed that a Netty EventLoopGroup getting wrapped byInstrumentedExecutorService will still cause a class cast exception in NettyHttpServer, thus we should continue to return the unwrapped instance as a precaution - that said I haven't found any Micronaut code that does such wrapping on EventLoopGroup now.

@jeremyg484 jeremyg484 closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants