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

[Feature] Support replacing rather than summing gauges #65

Open
Sinjo opened this issue May 3, 2023 · 4 comments · May be fixed by #77
Open

[Feature] Support replacing rather than summing gauges #65

Sinjo opened this issue May 3, 2023 · 4 comments · May be fixed by #77

Comments

@Sinjo
Copy link

Sinjo commented May 3, 2023

Hey 👋🏻

I've been looking at this project as one option for situations where scraping metrics is awkward (short-lived cron jobs in particular), but ran into one snag that's documented right at the top of the README:

Gauges are also added up (but this may not make any sense)

In many situations, summing up gauges doesn't make sense. The one I'm specifically running into is use of gauges to record the last time a job ran. In that situation, the behaviour I'd like is last-write-wins (i.e. whatever is currently in the gauge gets overwritten by a push to the same grouping key).

I was looking at the code, and it looks like it'd be fairly simple to make this behaviour conditional.

I'd be happy to send over a PR to do that. What I want to check is 1) whether you'd accept a PR that added a CLI flag to toggle the behaviour and 2) whether you have any preferences on implementation (e.g. should it be a global setting for all gauges in a given instance of the aggregation gateway).

@SpangleLabs
Copy link

Not sure if this is relevant, but while browsing the code for #5 I did notice that the aggregate functionality actually says gauges would be cleared out when read:
https://github.com/zapier/prom-aggregation-gateway/blame/main/metrics/merge.go#L86
Though, there's just a todo note in the code where that reset would be performed:
https://github.com/zapier/prom-aggregation-gateway/blame/main/metrics/aggregate.go#L149

So it seems like maybe the original intent was something inbetween replacing and summing? (Though I would definitely prefer if it were one or the other, rather than reading the metrics being a write operation, which definitely seems like it would be odd behaviour.)

Also, as a bystander, I feel like having the setting you suggest in question 2 would be much nicer as a per-gauge one, rather than a global one. There are definitely situations where a gauge might like to have one behaviour or the other, but I'm not sure how you would best delineate that

@SpangleLabs
Copy link

Of note also, is that the standard prometheus push gateway always implements the replace mechanic, one could theoretically use both if you want some metrics to replace with latest, and some to aggregate?

@Sinjo
Copy link
Author

Sinjo commented Jun 2, 2023

Yeah, it's something I've considered, but I'd rather just run one extra binary alongside any jobs that need to push metrics somewhere.

I'll leave this open, but having spoken to the maintainers of Vector on this issue thread, I'm going to go that route instead.

@SpangleLabs SpangleLabs linked a pull request Sep 1, 2023 that will close this issue
@tomj74
Copy link

tomj74 commented Oct 22, 2024

We are ready to adopt prom-aggregation-gateway in our project -- we are just waiting on this PR to be merged and included in a new release.

It's been a minute, any ETA on accepting the PR?
#77

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 a pull request may close this issue.

3 participants