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 target_info to registries #453

Merged
merged 1 commit into from
Sep 25, 2019
Merged

Add target_info to registries #453

merged 1 commit into from
Sep 25, 2019

Conversation

brian-brazil
Copy link
Contributor

This allows target labels as needed by push-based systems to be
provided, in a way that doesn't mess up Prometheus's own
top-down pull based approach to SD.

@SuperQ @RichiH @robskillington @sumeer Here's roughly what I'm thinking. Needs unittests etc.

@beorn7 Your input would be appreciated on the pgw front. prometheus/OpenMetrics#63 is the general background. Pgw could continue to do this out of band for grouping labels (potentially with the client library using this as a default), or have this as an option.

@beorn7
Copy link
Member

beorn7 commented Aug 13, 2019

I don't see a blocker from the Pushgateway side. There are many options how the PGW could incorporate this. If it's not the client library taking the target_info as a default for the grouping key, it could even be done by the PGW itself.


def _target_info_metric():
m = Metric('target', 'Target metadata', 'info')
m.add_sample('target_info', self._target_info, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, would it be required to also emit an actual value (i.e. in this case 1) or could we avoid that perhaps and special case it in the parser that metrics without a value are info metrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a standard info metric, so the value of 1 is required.

@brian-brazil brian-brazil marked this pull request as ready for review September 3, 2019 12:47
@brian-brazil
Copy link
Contributor Author

I've added unittests, and a basic check that we're not exporting two target_infos.

if labels:
if not self._target_info and 'target_info' in self._names_to_collectors:
raise ValueError('CollectorRegistry already contains a target_info metric')
self._names_to_collectors['target_info'] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused about this one, from my rudimentary understanding (or extreme rudimentary understanding should I say) - every value in _names_to_collectors should be a collector yeah?

If None is a value in _names_to_collectors and then it gets added to collectors var in restricted_registry:

        names = set(names)
        collectors = set()
        metrics = []
        with self._lock:
            for name in names:
                if name in self._names_to_collectors:
                    collectors.add(self._names_to_collectors[name])
            if 'target_info' in names and self._target_info:
                metrics.append(self._target_info_metric())
        for collector in collectors:
            for metric in collector.collect():
                samples = [s for s in metric.samples if s[0] in names]
                if samples:
                    m = Metric(metric.name, metric.documentation, metric.type)
                    m.samples = samples
                    metrics.append(m)

Wouldn't the collector.collect() be a call to None from the line for metric in collector.collect() if names happens to contain target_info?

Copy link
Contributor

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM other than my comments I left that are likely misguided, so thought would at least unblock you as you have a much better understanding than my great lack of understanding of these changes obviously

@robskillington
Copy link
Contributor

Hm, I was able to reproduce the issue I described in the comment I left:

>>> import prometheus_client
>>> registry = prometheus_client.CollectorRegistry()
>>> registry.set_target_info({'foo':'bar'})
>>> results = registry.restricted_registry(['target_info'])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "prometheus_client/registry.py", line 105, in restricted_registry
    for metric in collector.collect():
AttributeError: 'NoneType' object has no attribute 'collect'

This allows target labels as needed by push-based systems to be
provided, in a way that doesn't mess up Prometheus's own
top-down pull based approach to SD.

Signed-off-by: Brian Brazil <brian.brazil@robustperception.io>
@brian-brazil
Copy link
Contributor Author

Right you are, fixed and unittest added.

Copy link
Contributor

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM

@brian-brazil brian-brazil merged commit 7ced8d3 into master Sep 25, 2019
@brian-brazil brian-brazil deleted the targetinfo branch September 25, 2019 21:12
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.

3 participants