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

Fix downstream kamon instrumentation #1489

Merged

Conversation

hughsimpson
Copy link
Contributor

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)

Copy link
Member

@raboof raboof left a 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)
Copy link
Contributor

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.

Copy link
Contributor Author

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

@mdedetrich
Copy link
Contributor

I agree with the general change.

One concern might be that at some point, we might have better ways to implement metrics, at which point 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.

I'm all for removing @noinline once official hooks are added.

Copy link
Contributor

@pjfanning pjfanning left a 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.

@pjfanning pjfanning merged commit c3999a0 into apache:main Sep 21, 2024
9 checks passed
@hughsimpson hughsimpson deleted the fix_downstream_kamon_instrumentation branch September 21, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants