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

Figure out how to support target labels for push without breaking pull #63

Closed
brian-brazil opened this issue Oct 18, 2017 · 5 comments
Closed

Comments

@brian-brazil
Copy link
Contributor

In @sumeer's proposed proto the overall frame contains:

  repeated Label label = 1;  // Labels that apply to all metrics

These are called target labels in Prometheus, and it is considered a major anti-pattern to have target labels come from the thing being monitored rather than from service discovery+relabelling on the Prometheus end. If this ended up in the representation in the obvious way (apply it to all metrics), this would be very problematic for Prometheus.

For push based systems however, this information is vital. Prometheus also has one of these in the Pushgateway.

So we need some way for push systems to have this information, without it being added to every metric for the pull systems.

As a strawman I propose that the first metric of the output can be an annotation called target_info which contained this information. This would provide the information a push systems needs, while not messing up the data model of pull systems.

Another option would be to have this outside the format, which is what Prometheus currently does for these for the pushgateway. The target labels are passed to the function doing the pushing, but never in the format itself.

@sumeer
Copy link
Contributor

sumeer commented Oct 18, 2017

I would be ok with annotations. Not having it in the data model or wire format is not ideal -- that means the wire format would have vendor specific extensions.

@brian-brazil
Copy link
Contributor Author

Yeah, we need to specify it somehow as part of the spec as it's such a common thing, while not messing up pull.

@manolama
Copy link

Why would this mess up pull? If a user places a set of labels at the top of the hierarchy then they intend that these labels should be applied to all of the time series within the metric. Thus the receiving system should expand and apply them.

If someone is using Prometheus only code, then they wouldn't be populated, no?

@brian-brazil
Copy link
Contributor Author

brian-brazil commented Oct 21, 2017

If a user places a set of labels at the top of the hierarchy then they intend that these labels should be applied to all of the time series within the metric. Thus the receiving system should expand and apply them.

This is a common misconception with pull. The developer of the application/library is often not the person who is monitoring it, and thus cannot know what appropriate target labels are - e.g. they may not know if this is a dev or prod service. Or they do know, but a person doing horizontal monitoring explicitly only wants instrumentation labels now would have to figure out what all these arbitrary unwanted labels are to strip them off - which is an intractable problem when you get beyond a handful of services.

We also often see users wanting this to misuse this for things like version numbers, which are better handled as annotations.

We reject these feature requests every time we get them for the Prometheus client libraries, but that won't work so well for OpenMetrics so we need something that'll work for everyone.

If someone is using Prometheus only code, then they wouldn't be populated, no?

Any client library that's intended for OpenMetrics will have to offer this functionality somehow (even Prometheus needs it for our Pushgateway). If the data model were to be translated to the text representation in the obvious way (i.e. add those labels to all time series, as Prometheus requires the text format to be pre-flattened into our format) that'd be a problem.

@manolama
Copy link

The developer of the application/library is often not the person who is monitoring it, and thus cannot know what appropriate target labels are - e.g. they may not know if this is a dev or prod service.

True, which is why in the push model the agent can decorate the data with the appropriate tags to apply to all metrics coming from a host, etc.

We also often see users wanting this to misuse this for things like version numbers, which are better handled as annotations.

That's a documentation/encouragement issue, we shouldn't force them into a specific behavior.

If the data model were to be translated to the text representation in the obvious way (i.e. add those labels to all time series, as Prometheus requires the text format to be pre-flattened into our format) that'd be a problem.

For a new text format we could always have 'HEADER' lines with meta and common tags.

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

No branches or pull requests

4 participants