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

Restore metric collection for local calls. #429

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

ghemawat
Copy link
Collaborator

We had previously restricted metric collection to just remote calls for performance reasons. We now collect metrics for local calls as well since it was problematic to hide things like call counts for local calls.

This has a performance impact: on a typical machine, a local call slows down by ~100ns. By comparison, a local call slows down by 1000ns when tracing is enabled for that call. Future changes will attempt to speed up metric collection (we hope we can drop the penalty by 10x).

Changes the method metric names from "serviceweaver_remote_" to "serviceweaver_".

Include whether or not a call is remote in the metric label.

Updated the codegen metric collection API. Mainly, the responsibility for updating metrics is now in the codegen package as opposed to scattered throughout generated code.

Not updating the codegen version number since we have alread updated it since the last tagged release.

@ghemawat ghemawat requested a review from mwhittaker June 28, 2023 23:09
Copy link
Member

@mwhittaker mwhittaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

We had previously restricted metric collection to just remote calls
for performance reasons. We now collect metrics for local calls as
well since it was problematic to hide things like call counts for
local calls.

This has a performance impact: on a typical machine, a local call
slows down by ~100ns. By comparison, a local call slows down by 1000ns
when tracing is enabled for that call.  Future changes will attempt to
speed up metric collection (we hope we can drop the penalty by 10x).

Changes the method metric names from "serviceweaver_remote_*" to
"serviceweaver_*".

Include whether or not a call is remote in the metric label.

Updated the codegen metric collection API. Mainly, the responsibility
for updating metrics is now in the codegen package as opposed to
scattered throughout generated code.

Not updating the codegen version number since we have alread updated
it since the last tagged release.
@ghemawat ghemawat merged commit d30624e into ServiceWeaver:main Jun 29, 2023
7 checks passed
@ghemawat ghemawat deleted the metrics branch June 29, 2023 15:53
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.

None yet

2 participants