Replies: 12 comments
-
This is incorrect, when SD's have this, it is to append to the targets for scraping. |
Beta Was this translation helpful? Give feedback.
-
Many thanks for the correction. The SD documentation does indeed say this is the port for metric scraping (with the exception of Triton which says "The port to use for discovery and metric scraping") I couldn't find it stated explicitly in the documentation, but it turns out that the service discovery mechanisms all set an It was ec2_sd_config which confused me. How does it know whether you want to use
That is: it's hard-coded that
Thanks again for putting me right. Now I have to consider how this might interact with my proposal. I note that almost all the sd mechanisms which have a "port:" setting, default to port 80. The exceptions are:
To do this in a backwards-compatible way, we don't want the What I propose is:
At this point, it becomes more reasonable to suggest that the setting be called In summary:
People can then migrate over time. |
Beta Was this translation helpful? Give feedback.
-
That is not correct. Some SD's only use that when they can not determine which port to use (e.g. docker). |
Beta Was this translation helpful? Give feedback.
-
I think that this proposal over-complicates things. |
Beta Was this translation helpful? Give feedback.
-
Yes, I think this proposal as-is doesn't make the situation better. |
Beta Was this translation helpful? Give feedback.
-
An alternative would be to introduce shortcut actions in relabeling: |
Beta Was this translation helpful? Give feedback.
-
Are you referring to the backwards-compatibility approach I've suggested, or the ability to specify a target address without port in general? It seems odd to me that I can specify Another option is to go further, and say that
In this case |
Beta Was this translation helpful? Give feedback.
-
One of the main use cases of Prometheus is with kubernetes, where there are may pods on the same IP. In general, it is common to have the address and port in the same parameter, like we do for our --web.listen-address. I do not think we should start adding preferences and order here. |
Beta Was this translation helpful? Give feedback.
-
The rough consensus seems to go towards not accepting this idea. What do you all think about the "shortcut" relabel action: [action: remove-instance-port] ? Would it help addressing the initial issue @candlerb ? |
Beta Was this translation helpful? Give feedback.
-
Not really. The underlying problem is that the default instance label contains both address and port. This makes it very hard to join metrics between two different exporters on the same node. Once you've been bitten by this, you realise you have to throw away all your existing timeseries and do a whole load of relabelling, just to get "clean" instance labels. I think what you're suggesting is something like this:
as a magic shortcut for something like:
But whether you have to paste 2 extra lines into every scrape job or 5 extra lines doesn't really make much difference. It's the fact that you have to understand why this change is required, and to avoid being bitten by it in the first place. A secondary problem is that a ton of Grafana dashboards I find published out there are hard-coded to expect Now, I do realise there are cases where the port number is required to distinguish timeseries - for example if you are using docker SD (although that assumes that all the service ports which are exposed on a docker host are scrapable for metrics on the same path). In such cases, I agree that either it would be necessary for the instance label to include the port, or for a separate label to be present with the port, just to make sure the time series are distinct. But for cases like node_exporter and other node-level exporters, working at the level of the whole device or the whole VM, it would be really nice if the instance label were clean by default. It also seems odd to me that "port" belongs in If it has to be as it is, then so be it. It's just something I need to train people on, and it means introducing relabelling as a concept earlier than I would otherwise like. |
Beta Was this translation helpful? Give feedback.
-
Here is what I am doing: I am setting the |
Beta Was this translation helpful? Give feedback.
-
I do that too - and at that level of sophistication, relabelling is required to keep the targets file clean (*). Simpler setups use the DNS name as both instance label and target; and others may be happy to see the IP address as both instance and target. For those, it's either the DNS name or the IP address which identifies the target host. The port number is just a scraping detail, rather like the metrics path: its presence in the default instance label hampers the usefulness of the metrics. If you could set e.g. I guess what I'm suggesting could be implemented as an extra rule at the end of the relabel config, so let me try writing it as such:
Updating the address a single rule is a bit awkward given that IPv6 addresses can contain colons and RE2 doesn't do assertions, but maybe something like this will work (untested):
(*) although in principle you could do it without relabelling, using an ugly file sd with one target per "targets" block:
|
Beta Was this translation helpful? Give feedback.
-
Proposal
Add an optional
metrics_port
scrape config setting, which defaults to 0 (unset).At scrape time, if the target
__address__
doesn't end with colon-port, then:__metrics_port__
is set, use__metrics_port__
as the port numbermetrics_port
is set, usemetrics_port
__address__
as-is without any portThis is done after
__address__
is copied toinstance
(which of course only happens ifinstance
is unset)Use case. Why is this important?
Currently, scheme and path can be set globally for a job, but port cannot.
For newcomers to prometheus: they do the "obvious" thing which is to create targets files like this:
This results in two problems:
instance
label between them (because they embed two different port numbers)They then have to learn and deploy relabelling tricks for controlling the instance label, and modify their existing dashboards appropriately. If the instance label had just been the hostname in the first place, this wouldn't be necessary.
Experienced prometheus users, who know about controlling instance labels, end up writing verbiage in every scrape job just to eliminate the port number:
it would be so much simpler not to have to do this, and just set
metrics_port: 9100
instead.Backwards compatibility
This is entirely(*) backwards compatible. If you don't set
metrics_port
or__metrics_port__
then nothing changes. Even if you do,__address__
values containing port numbers continue to be permitted and will take precedence.I would expect that after a period of time, tutorials and examples would be updated to make use of
metrics_port
and remove the port numbers from targets files.(*) in theory, someone might have a relabelling config which uses
__metrics_port__
as an internal temporary variable; but the documentation says they should use labels starting__tmp
for this.Safety
I do not believe that allowing the port number to be dropped from the
__address__
(and hence the default instance label) introduces any problems. Firstly, this is good practice anyway. Secondly, ifmetrics_port
is set, this is at the job level. Every job has a uniquejob="<jobname>"
label, so two different scrapes with differentmetrics_port
will have distinct label sets.If you override using the
__metrics_port__
label then you can make collisions, but the same can be done with relabelling today. I suspect that__metrics_port__
will very rarely be used, in the same way as__scheme__
and__metrics_path__
, and is there really for completeness only. Given that the port can still be set in__address__
, then I wouldn't object strongly if__metrics_path__
were left out.Bikeshed
port
rather thanmetrics_port
??Many sd_configs have
port:
already,which of course is a different port (for talking to the sd mechanism, rather than scraping the target).Although the scrape_config could also gain aport
(likescheme
), on balance I think it is clearer to call it something else - for the same reason thatmetrics_path
is not simplypath
.Beta Was this translation helpful? Give feedback.
All reactions