Skip to content

Comments

[secretfilter] Add metrics to track the redaction of secrets#2985

Merged
dehaansa merged 16 commits intomainfrom
secret-filter-metrics
Apr 8, 2025
Merged

[secretfilter] Add metrics to track the redaction of secrets#2985
dehaansa merged 16 commits intomainfrom
secret-filter-metrics

Conversation

@kelnage
Copy link
Contributor

@kelnage kelnage commented Mar 13, 2025

PR Description

When the secretfilter component reacts secrets from log data, it is currently challenging to track how many secrets have been redacted, where they were redacted from, and how the component performs without expensive queries over Loki. This PR introduces multiple metrics to enable these to be tracked.

Which issue(s) this PR fixes

Notes to the Reviewer

I'd be very interested in your thoughts on the value of each of these metrics and my design choices around them. I would consider the loki_secretfilter_secrets_redacted_by_rule_total and loki_secretfilter_secrets_redacted_by_origin to be the key metrics - but I think each of the others would also have some value. Arguably loki_secretfilter_secrets_redacted_total could be determined directly from loki_secretfilter_secrets_redacted_by_rule_total - would it be better to just have the latter?

PR Checklist

  • Documentation added
  • Tests updated

@kelnage kelnage requested review from a team and clayton-cornell as code owners March 13, 2025 13:48
@kelnage kelnage requested a review from romain-gaillard March 13, 2025 13:48
@kelnage kelnage self-assigned this Mar 13, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2025

💻 Deploy preview deleted.

kelnage and others added 2 commits March 14, 2025 10:21
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@kelnage kelnage requested a review from a team March 14, 2025 10:40
@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Mar 14, 2025
@clayton-cornell
Copy link
Contributor

Docs look OK for now. I'll wait on final approve for docs until after code review.

Copy link
Contributor

@wildum wildum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this and for all the tests, this is great!
Only two small remarks:

  • the changelog entry is missing
  • it would be nice to have a little bit more test coverage for the update behavior in the tests: ensuring that existing metrics are not reset between two updates + what happens if I update with a different origin label and then update back to the previous origin label? Will I start from the previous value or from 0?

@CLAassistant
Copy link

CLAassistant commented Apr 2, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@kelnage
Copy link
Contributor Author

kelnage commented Apr 3, 2025

@wildum hope the new test matches your expectations - helpfully it also revealed a bug with updating the allowlists (it didn't clear the old allowlist) 👍 The failing tests seem completely unrelated to my changes post-merging in main, and are possibly just flakey tests.

@dehaansa
Copy link
Contributor

dehaansa commented Apr 3, 2025

Re-running the test to confirm flakiness, and we'll need an updated changelog as this just missed the RC for 1.8.

kelnage and others added 3 commits April 8, 2025 11:02
@dehaansa dehaansa merged commit 537e036 into main Apr 8, 2025
35 checks passed
@dehaansa dehaansa deleted the secret-filter-metrics branch April 8, 2025 20:32
matthewnolf pushed a commit that referenced this pull request Apr 9, 2025
* Initial implementation of secretfilters metrics

* Add summary metric for performance monitoring

* Update label metrics to use user-specified list

* Update Loki label metric to a single origin label

Necessary as otherwise updating the labels being tracked would lead to
prometheus creating multiple metrics with different labels to track.

* Update documentation to reflect new metrics

* Apply docs suggestions from code review

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>

* Revert change to bullet points style

* Validate behavior from repeated changes via Update

* Add changelog entry for secretfilter metrics

* Move changelog entry to correct heading

* Apply suggestions from code review on docs

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>

* Revert unnecessary formating changes

---------

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

frozen-due-to-age type/docs Docs Squad label across all Grafana Labs repos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants