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

chore: use HumanizeTimestamp from prometheus/common #3924

Conversation

freak12techno
Copy link
Contributor

Adding HumanizeDuration from prometheus/common (moved it there in this PR: prometheus/common#654).
Also copy-pasted some test cases from Prometheus repo.

@freak12techno
Copy link
Contributor Author

@gotjosh since it was you who reviewed my last PR here, can you check or ask someone to do so?

@freak12techno
Copy link
Contributor Author

Seems like the test failing is not mine and should've been caused by my PR right?

@simonpasquier
Copy link
Member

Thanks for PR! I believe that the PR should be retitled "chore: use HumanizeTimestamp from prometheus/common". Also please add the new humanizeTimestamp function to docs/notifications.md.

I'm not quite sure where someone would have the opportunity to use the function in Alertmanager templates?

@freak12techno freak12techno changed the title chore: use HumanizeDuration from prometheus/common chore: use HumanizeTimestamp from prometheus/common Jul 26, 2024
@freak12techno
Copy link
Contributor Author

@simonpasquier thanks for noticing the PR naming, I updated it. On docs, will also add it a bit later.
For usage, I personally have some alerts that expose unix timestamp as a value, would be nice to be able to format it in a nice way.

@freak12techno
Copy link
Contributor Author

@simonpasquier fixed the docs as well

@simonpasquier
Copy link
Member

For usage, I personally have some alerts that expose unix timestamp as a value, would be nice to be able to format it in a nice way.

Got it but the value shouldn't be present in the data accessible to the Alertmanager templates. Or am I missing something?

@freak12techno
Copy link
Contributor Author

@simonpasquier actually yes, haven't thought of that. Do you think it'll be still useful or if I should close it?

@simonpasquier
Copy link
Member

I wouldn't add the function until we have a use case (otherwise it creates unnecessary load for users and maintainers).

@freak12techno
Copy link
Contributor Author

Gotcha, I'll close it then. Will re-add if there's a usecase for it later.

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.

2 participants