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

Added duration in seconds as logger attribute #318

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

or-shachar
Copy link

I'm trying to monitor reconciliation durations using the structured log objects, and I found it hard to extract it out of the plain message. I suggest adding another field to the log object with durationSeconds that can be easily turned into a metric.

I'm not quite sure how to test this new code though, seems like the logging is uncovered by the tests anyhow.
Any advice would be appreciated!

@or-shachar or-shachar marked this pull request as ready for review April 28, 2022 09:08
@or-shachar
Copy link
Author

@mvoitko Sorry for the late response. Marked this as ready to review and rebased the branch on top of main.

@stefanprodan
Copy link
Member

stefanprodan commented Apr 28, 2022

We have a Prometheus histogram for this, please see the docs here https://fluxcd.io/docs/guides/monitoring/#metrics

If we decide to add the durationSeconds to structured logs, then this must be done across all Flux controllers.

@or-shachar
Copy link
Author

@stefanprodan - Actually when I opened it we didn't have prometheus scraper within the cluster but only logs scraper.
We used a different solution for metric reporting.

Now we do use Prometheus so it's not that important for us and indeed we use that historgram.

Do you think it's still valuable to do it across all Flux controllers? The pro is that it allows collecting data even without Prometheus. I don't know if our use case was unique or not.

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.

None yet

3 participants