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

Implement Prometheus Counter type #27

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

thdebay
Copy link

@thdebay thdebay commented Jan 31, 2024

This PR adds the ability to create metrics of type counter (see https://prometheus.io/docs/concepts/metric_types/#counter)

To use it, create the counter with

$counter = Prometheus::addCounter('my counter');

optionally define an initial value like so:

$counter = Prometheus::addCounter('my counter')->setInitialValue(123);

and increment it with

$counter->inc();

optionally pass a value to increment the counter by more than one:

$counter->inc(2);

@freekmurze
Copy link
Member

Nice! Could you also document this?

@thdebay
Copy link
Author

thdebay commented Jan 31, 2024

Absolutely, I'll be happy to document it.

I'm realizing, however, that the benefits of counters are limited when using the in-memory storage. To take advantage of counters with PHP applications I think some caching is needed and it usually works great with the APC or APCng storage collector registries.

Is it a config option you're planning to implement some day? It could be part of this PR if that seems interesting to you, I guess it could be an additional option in the config file

@pselge-5anker
Copy link
Contributor

The idea of using APCu/APCng sounds great.

I suggest writing a laravel adapter, that implements the Prometheus/Storage/Adapter Interface from PromPHP/prometheus_client_php. This adapter receives a selectable Illuminate\Contracts\Cache\Store (configured via config/cache.php; referenced via config/prometheus.php).

This way we would have ootb access to database, redis, memcache, dynamodb, apc, apcu, array and null storages and benefit from third party libraries that add custom storages. It would also bring in all of the test functions that are provided by the laravel caching facades.

Separating common storages between nodes could happen through individual host tagging.

@pselge-5anker
Copy link
Contributor

I provided a laravel cache adapter in #32. @thdebay maybe you can check if this works for you. In my manual tests the initial value of your counter was added each time i called it, which makes sense for the implementation but it might be easier to understand, if it would not be called initialValue.

@thdebay
Copy link
Author

thdebay commented Mar 10, 2024

@pselge-5anker Did you manage to make the counter metrics work with your cache adapter implementation?

I think your PR looks great, however, in practice I'm still struggling to make a successful test. When retrieving the metrics to render, the values are empty as if the counter was just initialized.

I'll keep looking into it, but I'm curious to know if you managed to make it work

@pselge-5anker
Copy link
Contributor

@thdebay Yes, it worked fine. I tried it with the database, file, redis and array adapter. the latter of course lost its state after each call while the others kept increasing with each request i sent.
I think configuring file is the easiest to quickly test it, as you can tail the cache files.

have you configured the adapter in both config/queue.php and config/prometheus.php?

@thdebay
Copy link
Author

thdebay commented Mar 10, 2024

I guess I still don't figure out how the $collectors property in the Prometheus class are supposed to go into the cache when creating a counter from outside the PrometheusServiceProvider. It works if I create the counter there, but not when I call it from another controller, for instance.

@pselge-5anker
Copy link
Contributor

I will have a look tomorrow morning. I'm not sure if I conducted any tests outside of the ServiceProvider so far. I was trying to bring the CacheAdapter into the php_prometheus repo, to have it managed further up the chain, as I believe it to be beneficial to other libraries as well.
The tests over there checked out as well. Maybe It's not working all too well with the facade. I'll post my findings tomorrow.

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

3 participants