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

Remove finalizers #115

Merged
merged 31 commits into from Jun 10, 2022
Merged

Remove finalizers #115

merged 31 commits into from Jun 10, 2022

Conversation

stone-z
Copy link
Contributor

@stone-z stone-z commented Jun 1, 2022

To support sharding metrics across exporters, it would be nice to get rid of the finalizers added by each controller to their watched aqua resources. The finalizers were only needed because it isn't possible to delete a metric from the golang prometheus client without specifying all of the labels for that metric -- in other words, we had to reconstruct the entire metric (so vulnerability details, etc.) in order to delete it. Finalizers were added to give the exporters an opportunity to clear the metrics before the report is deleted.

I've since implemented the functionality needed in the prometheus client, but this change has not been released yet.

With sharded metrics, the shard owner is responsible for clearing the finalizer after it has cleared its metrics for a given report. However, even the finalizer does not handle all cases where we might miss metrics, for example, when a statefulset is updated as described by #48. So, I'm committing a minor sin by using an untagged version of the prometheus client in order to a. solve the issue with stale metrics and b. hopefully get us closer to caching fewer reports per instance.

Checklist

  • Update changelog in CHANGELOG.md.
  • Make sure values.yaml and values.schema.json are valid.
  • (Giant Swarm) If creating a release, bump the version and appVersion in Chart.yaml.

@stone-z stone-z changed the title Try untagged prom client Remove finalizers Jun 8, 2022
@stone-z stone-z self-assigned this Jun 8, 2022
@stone-z stone-z requested a review from fhielpos June 8, 2022 14:30
Copy link
Member

@fhielpos fhielpos left a comment

Choose a reason for hiding this comment

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

Nice 🚀

@stone-z
Copy link
Contributor Author

stone-z commented Jun 9, 2022

@giantswarm/team-atlas as our resident metric gurus, if one of you has a quick moment today/tomorrow to see if any of this is completely backwards I'd appreciate it. I'll merge this tomorrow

@stone-z stone-z marked this pull request as ready for review June 9, 2022 19:05
@stone-z stone-z merged commit 82845f0 into main Jun 10, 2022
@stone-z stone-z deleted the use-untagged-promclient branch June 10, 2022 14:40
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

2 participants