Add option to set the value to helm release's timestamp #36
Conversation
|
If changing the value to a time series what would you be graphing in grafana? Do you have an example of what you are trying to accomplish? |
So, what I am trying to achieve is deployment annotations in grafana, that use the metrics exported by helm-exporter. In grafana the are two ways to achive this:
Where 1590478033 is the unix representation of the time the deployment occurred. |
|
Currently, helm-exporter is providing a timeseries/metric like this: This causes grafana to draw a vertical line for every minute, assuming that a new deployment has been occurring every minute. I have found no PromQL query that can transform to I hope this helps explain the issue. If not I can provide more info. Thanks! |
|
we have an annotation for helm sync with
|
|
Excellent that's a great idea. So with your PR we have two modes.
Would it make sense to not make them mutually exclusive? Maybe a default mode where metrics for both get generated and a flag to turn each of them off separately? |
|
@sstarcher, I don't think it makes much sense to have both options enabled simultaneously. I mean what is the point of having the same metrics twice with different values? Is that something you think must be done in order to merge this PR? If not can you merge it? |
|
But it's not the same metric. I would recommend renaming it. One allows you to track the status of a release. The one you added allows you to track release events. My main use case is tracking the current and prior status of a release. With the current code if I enable your option to track events I can no longer track release statuses. |
This is needed in order to use the helm_chart_info metric for Grafana annotations.
fccf90a to
8341bc7
Compare
| statsInfo.WithLabelValues(chart, releaseName, version, appVersion, strconv.FormatInt(updated, 10), namespace, latestVersion).Set(status) | ||
| } | ||
| if *timestampSeries == true { | ||
| statsTimestamp.WithLabelValues(chart, releaseName, version, appVersion, strconv.FormatInt(updated, 10), namespace, latestVersion).Set(float64(updated)) |
There was a problem hiding this comment.
Now that we have two metrics do we need to duplicate all of the information? Or could statsInfo contain all of the information like it has had and statsTimestamp contains just the things needed to make it reference statsInfo such as release and namespace?
There was a problem hiding this comment.
🤔 you have requested that we make each metric optional, so what if a user only enables one of them?
There was a problem hiding this comment.
ya, with my above suggestion making them optional would not really make sense. I think it would be best to have the existing one always one with statsTimestamp optional, but I'm also fine with how you have it now.
There was a problem hiding this comment.
I'll leave it as it is then.
- Add a second metric for timestamps - Make info and timestamp metrics not mutually exclusive - Add flags to turn off info and timestamp metrics
8341bc7 to
b1ad066
Compare
|
@sstarcher I have spent some time testing this PR and it seems to be working nicely now (after fixing a minor naming bug). Could you have another look and let me know if you would like anything else? |
|
Thanks for your contributions! |
|
Bah, we should have updated the README.md to add info about the new metric. |
add option to set the value to helm release's timestamp
Recent versions of Grafana (I am testing with v6.7.1) have a "Series value as timestamp" option which can be used with the timestamp option implemented in this PR, in order to use the helm_chart_info metric for Grafana annotations pulled from Prometheus.
https://grafana.com/docs/grafana/latest/reference/annotations/#querying-other-data-sources