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

Prometheus stuff #301

Merged
merged 4 commits into from
Oct 17, 2023
Merged

Prometheus stuff #301

merged 4 commits into from
Oct 17, 2023

Conversation

coolreader18
Copy link
Collaborator

Description of Changes

Sorry, still in a bit of a funk, but figured I may as well just push what I have.

API and ABI

  • This is a breaking change to the module ABI
  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

Copy link
Contributor

@kim kim left a comment

Choose a reason for hiding this comment

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

This seems like an improvement overall, but I'd appreciate a bit of tidying.

We'll also need a follow-up to integrate into cloud.

use spacetimedb_lib::{Address, Hash, Identity};

#[macro_export]
macro_rules! metrics_group {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nice.

But also, possibly more generally useful than deep down the core crate? There is also a prometheus-macros crate on crates.io, maybe there is some overlap?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh wow, there definitely is. That's funny lol, we had almost the same idea. I think in the future I'll look at expanding it to something more general (unsure about pulling it out into its own crate that I then publish or something, wrt licensing) but the one on crates.io doesn't let you make fields public (??) or else I might consider switching. I'm happy with the strong typing with this implementation, as well (not just strings everywhere).

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant maybe a workspace crate, or in lib, or somewhere near the top of the core crate module hierarchy. Just so me and others can find it easily for instrumenting other stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, moved it to a submodule of spacetimedb_core::util

crates/core/src/util/prometheus_handle.rs Show resolved Hide resolved
crates/core/src/util/prometheus_handle.rs Show resolved Hide resolved
crates/core/src/util/lending_pool.rs Outdated Show resolved Hide resolved
crates/core/src/util/lending_pool.rs Outdated Show resolved Hide resolved
crates/core/src/database_logger.rs Show resolved Hide resolved
crates/core/src/json/client_api.rs Outdated Show resolved Hide resolved
crates/core/src/auth/identity.rs Show resolved Hide resolved
@coolreader18 coolreader18 force-pushed the noa/prometheus-stuff branch 3 times, most recently from 101dd4e to efe869f Compare October 16, 2023 21:29
@drogus
Copy link
Collaborator

drogus commented Oct 17, 2023

I just wanted to say that I've been running with this changes on dev for a week or so and observing some of the metrics (like the queue length) and it works without any issues. Thanks for great work Noa!

Also it would be great to merge it before 0.7.1, cause it's very useful for observing the load on the database, especially the module calls contention.

@coolreader18 coolreader18 enabled auto-merge (squash) October 17, 2023 18:41
Copy link
Collaborator

@drogus drogus left a comment

Choose a reason for hiding this comment

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

Great work!

@coolreader18 coolreader18 merged commit ac24ff0 into master Oct 17, 2023
5 checks passed
@drogus drogus deleted the noa/prometheus-stuff branch October 17, 2023 19:05
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