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.

Sign up for GitHub

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

Jump to bottom

feat: observability support for JVM #3051

Merged
merged 1 commit into from
Oct 10, 2024
Merged

feat: observability support for JVM #3051

merged 1 commit into from
Oct 10, 2024

Conversation

stuartwdouglas
Copy link
Collaborator

fixes: #3041

@stuartwdouglas stuartwdouglas requested review from a team as code owners October 9, 2024 06:11
@stuartwdouglas stuartwdouglas requested review from alecthomas and tomdaffurn and removed request for a team October 9, 2024 06:11
This was referenced Oct 9, 2024
Open
Open
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/3041 branch 2 times, most recently from 2311664 to 11fb717 Compare October 9, 2024 20:28
jonathanj-square
jonathanj-square approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@jonathanj-square jonathanj-square left a comment

Choose a reason for hiding this comment

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

looks good overall, minor naming request added

jvm-runtime/ftl-runtime/common/runtime/src/main/java/xyz/block/ftl/runtime/VerbHandler.java Outdated
this.registry = registry;
counter = meter.counterBuilder("ftl-jvm-runtime/verb_invocations")
Copy link
Contributor

Choose a reason for hiding this comment

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

this appears to mirror:

ftl/backend/controller/observability/calls.go

Line 44 in 8453735

signalName := fmt.Sprintf("%s.requests", callMeterName)

can you use the same naming scheme

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually wonder if we need this at all, given that we already track request numbers in the controller, it was more added just to make sure the metrics were working.

I might replace it with some user level metrics in the examples.

@stuartwdouglas stuartwdouglas requested review from a team and wesbillman and removed request for a team October 9, 2024 22:28
@stuartwdouglas stuartwdouglas merged commit db0ca7f into main Oct 10, 2024
95 checks passed
@stuartwdouglas stuartwdouglas deleted the stuartwdouglas/3041 branch October 10, 2024 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathanj-square jonathanj-square jonathanj-square approved these changes

@tomdaffurn tomdaffurn tomdaffurn approved these changes

@alecthomas alecthomas Awaiting requested review from alecthomas alecthomas is a code owner automatically assigned from TBD54566975/ftl-infra-team

@wesbillman wesbillman Awaiting requested review from wesbillman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Observability support for JVM
3 participants
@stuartwdouglas @tomdaffurn @jonathanj-square