search

endclothing / prometheus_client_php

Prometheus instrumentation library for PHP applications
https://prometheus.io/docs/concepts/metric_types/
Apache License 2.0
150 stars 79 forks source link

Inject Redis via named constructor #8

Closed martinssipenko closed 4 years ago

martinssipenko commented 5 years ago

This PR adds ability to create Redis storage class by injecting existing Redis connection.

Clsoes #7

martinssipenko commented 5 years ago

If you are happy with this way of doing it I'll add tests.

NoelDavies commented 5 years ago

If you are happy with this way of doing it I'll add tests.

I personally think this works! I'd suggest in the future you write tests first, allowing you to design your API first.

But looks good, just add tests. Nice addition to the project 👍

martinssipenko commented 5 years ago

@NoelDavies while trying to integrate my forked branch into my project I noticed that when I inject Redis connection with set prefix, metrics stop working. After poking around for a bit I discovered that PhpRedis adds a prefix to KEYS only, and not ARGS. As a result, before my changes, if prefix used lead to redis commands being prefixed too, which lead to errors.

I added tests to cover this case and afterwards changed the KEYS and ARGS order. Now this works with and without redis prefix, all tests pass.

I also added an extra check in Redis::fromExistingConnection() method so that if connection is not established exception is thrown.

I hope this can be released soon.

piotrooo commented 5 years ago

You know, in this implementation you are close only for one of the Redis implementation and it's breaks an inverse of control pattern.

This implementation should be a default Redis implementation, which implement some RedisClientIntreface, that allows easly extend Redis client implementation by the dev which uses diffrent Redis client.

martinssipenko commented 5 years ago

@piotrooo I can't disagree, however this was not the intention of this pull request. Given implementation already is based on PhpRedis \Redis class this PR only adds ability to inject same implementation from outside. Perhaps this could be discussed and changed in new major version.

piotrooo commented 5 years ago

@martinssipenko sounds reasonably. Should be discussed and reimplemented - IMHO.

martinssipenko commented 5 years ago

@NoelDavies any updates on this?

martinssipenko commented 4 years ago

@NoelDavies any updates on this?

martinssipenko commented 4 years ago

@NoelDavies I think you were looking at outdated version of code, all feedback that was given has been implemented - https://github.com/endclothing/prometheus_client_php/pull/8/files

NoelDavies commented 4 years ago

Can we rebase from master to get the CI changes ?

martinssipenko commented 4 years ago

Done, this branch is now up to date with upstream master. However, the CI is still failing because of incompatible Guzzle version, that I fixed in #21. If you want I can fix the CI here or in a different branch forked from current master.

NoelDavies commented 4 years ago

I merged #21 now, it should run the CI fine now?

martinssipenko commented 4 years ago

Rebased again, CI should be green now, also other PRs should be rebased and be green before merging (you can require that CI passes before merge is allowed in githubs branch protection settings).