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

Add tags support to GraphiteBridge #618

Merged
merged 3 commits into from
Feb 4, 2021

Conversation

partcyborg
Copy link
Contributor

Graphite has support for tagged metrics with a syntax very similar to
the non-tagged format. Update GraphiteBridge to support it.

Graphite's carbon tags format is explained here: https://grafana.com/blog/2018/01/11/graphite-1.1-teaching-an-old-dog-new-tricks

@brian-brazil

Graphite has support for tagged metrics with a syntax very similar to
the non-tagged format.  Update GraphiteBridge to support it.

Signed-off-by: Matt Wilder <me@partcyb.org>
@partcyborg partcyborg force-pushed the graphite_bridge_tags branch from b0ca484 to 1041d23 Compare February 3, 2021 02:23
@partcyborg
Copy link
Contributor Author

@csmarchbanks (new project maintainer)

Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

A couple of small comments, but otherwise this looks good to me.

README.md Outdated Show resolved Hide resolved
prometheus_client/bridge/graphite.py Outdated Show resolved Hide resolved
README.md Outdated
```python
from prometheus_client.bridge.graphite import GraphiteBridge

gb = GraphiteBridge('graphite.your.org', 2003, tags=True)
Copy link
Member

Choose a reason for hiding this comment

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

This should be GraphiteBridge(('graphite.your.org', 2003), tags=True) for the same reason as the last comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

* Fix `GraphiteBridge` calls in README.md

* Place the new `tags` kwarg at the end of the `GraphiteBridge`
  constructor.

Signed-off-by: Matt Wilder <me@partcyb.org>
README.md Outdated
```python
from prometheus_client.bridge.graphite import GraphiteBridge

gb = GraphiteBridge(('graphite.your.org', 2003, tags=True))
Copy link
Member

Choose a reason for hiding this comment

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

Too many things in the tuple here :)

It should be GraphiteBridge(('graphite.your.org', 2003), tags=True)

Signed-off-by: Matt Wilder <me@partcyb.org>
@partcyborg partcyborg force-pushed the graphite_bridge_tags branch from 832f702 to 7f83b9e Compare February 4, 2021 22:22
Copy link
Member

@csmarchbanks csmarchbanks 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, thanks!

@csmarchbanks csmarchbanks merged commit ccb8395 into prometheus:master Feb 4, 2021
@partcyborg partcyborg deleted the graphite_bridge_tags branch February 4, 2021 22:33
@partcyborg
Copy link
Contributor Author

Thanks for the quick review!

Do you have a general idea of when a prometheus_client release with this change might make its way to pypi? It would just be nice to know when I stop building this from the git repo in our relevant Dockerfiles.

@csmarchbanks
Copy link
Member

Good question, my estimate is within a month or so. As you saw, I recently took over as maintainer of this repo and am still on-boarding some.

@partcyborg
Copy link
Contributor Author

Sounds good. Thanks again and good luck with maintainership.

cVoltic pushed a commit to cVoltic/client_python that referenced this pull request Feb 8, 2021
Add tags support to GraphiteBridge

Graphite has support for tagged metrics with a syntax very similar to
the non-tagged format.  Update GraphiteBridge to support it.

Signed-off-by: Matt Wilder <me@partcyb.org>
Signed-off-by: Trinh <iy3827-admin@dupont.com>
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.

2 participants