Skip to content

Assert label values be str or unicode#177

Closed
kehao95 wants to merge 1 commit intoprometheus:masterfrom
kehao95:master
Closed

Assert label values be str or unicode#177
kehao95 wants to merge 1 commit intoprometheus:masterfrom
kehao95:master

Conversation

@kehao95
Copy link

@kehao95 kehao95 commented Jul 12, 2017 •
edited
Loading

Right now, if you call add_metric() method with no string variables. Nothing will happen. But will raise meanless AttributError when first scrape.

quick reproduce

import time
from prometheus_client import start_http_server
from prometheus_client.core import GaugeMetricFamily, REGISTRY

class CustomCollector(object):
    def collect(self):
        foo = GaugeMetricFamily('foo_bar', '', labels=['label1', 'label2'])
        foo.add_metric([1, 'bar'], 1) # Use an int as label value
        yield foo

REGISTRY.register(CustomCollector())

if __name__ == '__main__':
    start_http_server(9091)
    while True:
        time.sleep(30)

When you try to scrape the metrics, it will raise a helpless Tracback.

----------------------------------------
Exception happened during processing of request from ('127.0.0.1', 64728)
----------------------------------------
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/socketserver.py", line 318, in _handle_request_noblock
    self.process_request(request, client_address)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/socketserver.py", line 344, in process_request
    self.finish_request(request, client_address)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/socketserver.py", line 357, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/socketserver.py", line 684, in __init__
    self.handle()
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/http/server.py", line 415, in handle
    self.handle_one_request()
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/http/server.py", line 403, in handle_one_request
    method()
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/prometheus_client/exposition.py", line 86, in do_GET
    output = generate_latest(registry)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/prometheus_client/exposition.py", line 72, in generate_latest
    for k, v in sorted(labels.items())]))
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/prometheus_client/exposition.py", line 72, in <listcomp>
    for k, v in sorted(labels.items())]))
AttributeError: 'int' object has no attribute 'replace'

@kehao95
Copy link
Author

kehao95 commented Jul 12, 2017

After patch:
The error rasied the moment you call add_metric().

Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/prometheus_client/core.py", line 228, in add_metric
    assert type(label) is str
AssertionError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/kehao/code/Auryc/DockerizedApps/Monitoring/elb_exporter/test.py", line 11, in <module>
    REGISTRY.register(CustomCollector())
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/prometheus_client/core.py", line 50, in register
    names = self._get_names(collector)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/prometheus_client/core.py", line 86, in _get_names
    for metric in desc_func():
  File "/Users/kehao/code/Auryc/DockerizedApps/Monitoring/elb_exporter/test.py", line 8, in collect
    foo.add_metric([1, 'bar'], 1)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/prometheus_client/core.py", line 230, in add_metric
    raise TypeError("label values must be 'str' not %r"% type(label).__name__)
TypeError: label values must be 'str' not 'int'

@kehao95 kehao95 force-pushed the master branch 2 times, most recently from df8959c to 4831c4c Compare July 12, 2017 07:07
@kehao95 kehao95 changed the title Assert label values be str Assert label values be str or unicode Jul 12, 2017
brian-brazil
brian-brazil reviewed Jul 12, 2017
View reviewed changes
prometheus_client/core.py
labels: A list of label values
value: The value of the metric.
'''
try:
Copy link
Contributor

@brian-brazil brian-brazil Jul 12, 2017

Choose a reason for hiding this comment

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

It'd be more Pythonic to duck type and to convert a string, rather than limit to two particular classes.

This needs a unittest

Copy link
Author

@kehao95 kehao95 Jul 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ah yes, I wish I could think up a more Pythonic way.
I found this Question in StackOverflow
https://stackoverflow.com/questions/11301138/how-to-check-if-variable-is-string-with-python-2-and-3-compatibility
The best one needs six. I have no idea.

Copy link
Author

@kehao95 kehao95 Jul 12, 2017

Choose a reason for hiding this comment

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

I reviewed the string_types definition in the six project.
GitHub-benjaminp/six

if PY3:
    string_types = str,
    integer_types = int,
    class_types = type,
    text_type = str
    binary_type = bytes

    MAXSIZE = sys.maxsize
else:
    string_types = basestring,
    integer_types = (int, long)
    class_types = (type, types.ClassType)
    text_type = unicode
    binary_type = str

Copy link
Contributor

@brian-brazil brian-brazil Jul 12, 2017

Choose a reason for hiding this comment

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

Technically what we need here is a utf8 string.

@brian-brazil
Copy link
Contributor

brian-brazil commented Mar 22, 2018

This is stale.

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

Reviewers

1 more reviewer

@brian-brazil brian-brazil brian-brazil 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.

2 participants

@kehao95 @brian-brazil