-
Notifications
You must be signed in to change notification settings - Fork 150
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
Fix downstream kamon instrumentation #1489
Fix downstream kamon instrumentation #1489
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the general change.
One concern might be that at some point, we might have better ways to implement metrics, at which time we might be asking ourselves "is it ok to remove this @noinline
?" - which is hard to answer without more specific references to exactly which instrumentations rely on these methods. On the other hand, if we'd add such references there's a high chance they'd go stale as well - so this is probably fine.
queueFactory: ThreadPoolConfig.QueueFactory = queueFactory, | ||
rejectionPolicy: RejectedExecutionHandler = rejectionPolicy | ||
): ThreadPoolConfig = | ||
ThreadPoolConfig(allowCorePoolTimeout, corePoolSize, maxPoolSize, threadTimeout, queueFactory, rejectionPolicy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a little bit of time on this yesterday and I got it wrong - I thought that this would only work if it was declared with override
because in theory this function overrides the default copy
function that gets added to case classes.
I still think it is quite odd to instrument the copy function as a way to instrument pinned dispatchers. I raised kamon-io/Kamon#1366 about seeing if there is a better place to instrument this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this one left a slightly bad taste in my mouth and there probably is a better way. Ultimately I'd rather focus improvements on exposing deliberate instrumentation hooks from within pekko and changing the downstream to use them, because otherwise there's an inherent fragility... Any weaving approach can only work if there's a stable non-inlined API that's deliberately left for that purpose -- there's almost certainly places where we're currently only passing because of the whims of the scala compiler. However, such an approach will require considerably more time and thought. Since the Kamon library is probably one of the more widely used ways of instrumenting pekko, I think this is helpful as a stopgap that doesn't block people from adopting 1.1.x; but I completely understand your point of view here
I'm all for removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - I'm happy to add as is but would like to see if the ThreadPoolConfig.copy can be replaced as an instrumentation point in Kamon. This doesn't need to block merge of this PR and we can undo the ThreadPoolConfig.copy change later if we no longer need it.
cf #1484 and kamon-io/Kamon#1352
These three sites are enough to fix downstream kamon instrumentation (at least, enough to pass the test suite and negate the need for a recent workaround)