Skip to content
This repository was archived by the owner on Oct 22, 2019. It is now read-only.

Add Predis support #55

Open
fesor wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add Predis support #55

fesor wants to merge 3 commits into from

Conversation

@fesor
Copy link

@fesor fesor commented Sep 18, 2017

For #52

@fesor
Copy link
Author

fesor commented Nov 5, 2017 •
edited
Loading

@schnipseljagd @bracki guys, what do you think on this?

@bracki
Copy link
Contributor

bracki commented Nov 6, 2017

I'd prefer to refactor the class so that it takes either a Redis or a Predis $connection, rather than duplicating everything. @fesor what's your take on that?

@fesor
Copy link
Author

fesor commented Nov 6, 2017

Will try to do that.

@fesor
Introduce redis client adapter
e2d1074 
 - Encapsulate differences between predis and redis behind
single abstraction.
 - Reduce duplication in tests
@fesor
Copy link
Author

fesor commented Feb 26, 2018 •
edited
Loading

@bracki so, I need this again so I finally managed to work on this PR.

I added additional abstraction which hides difference between redis and predis without sacrifice lazy connection in case of redis.

I really not sure it is ok or not.

@bracki
Copy link
Contributor

bracki commented Mar 19, 2018

@fesor Would you mind rebasing? Did you also take into account #27?

bracki
bracki reviewed Mar 19, 2018
View reviewed changes
src/Prometheus/PushGateway.php
* @param $groupingKey
*/
public function delete($job, $groupingKey = null)
public function delete(CollectorRegistry $collectorRegistry, $job, $groupingKey = null)
Copy link
Contributor

@bracki bracki Mar 19, 2018

Choose a reason for hiding this comment

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

$collectorRegistry is left out on purpose here. It's not being used anyways. Modeled after the Python implementation which also leaves out the registry when doing a delete.

@fesor
Copy link
Author

fesor commented Mar 20, 2018

@bracki as for #27, yes and no. You can pass redis instance but adapter still will handle connection and will require connection options.

I need to check is this covers #27 fully.

Willl rebase this PR asap.

@peterjumpnl
Copy link

peterjumpnl commented Sep 25, 2018

What is status of this pr?

@MyIgel
Copy link

MyIgel commented Jun 26, 2019

Is there anything I could help with this PR to get merged? This would be a really nice feature :(

@NoelDavies
Copy link

NoelDavies commented Oct 20, 2019

This project is dead, but I'm maintaining it under my employer - https://github.com/endclothing/prometheus_client_php. Feel free to submit the PR there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@bracki bracki bracki left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fesor @bracki @peterjumpnl @MyIgel @NoelDavies