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

feat(kuma-cp): Resource counter based on Mesh insights #1423

Merged

Conversation

jewertow
Copy link
Contributor

Signed-off-by: Jacek Ewertowski jacek.ewertowski1@gmail.com

Issues resolved

Fix #1165

@jakubdyszkiewicz please check if it is what you expected?

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>
@lobkovilya
Copy link
Contributor

Hey @jewertow! You're on the right track, don't hesitate to move the PR from draft.

There is one moment that concerns me, MeshInsight contains information only about mesh-scoped resources, but the previous implementation of count() took into account all resources. It'd be great to cover the existing behavior of count with some simple test at first and then make sure your code passes it.

@jewertow
Copy link
Contributor Author

@lobkovilya Yes, you're right that MeshInsight contains information only about mesh-scoped resources, but I looked how the metric "resource_count" is used and I found only two usages:

  1. sum by (zone, instance) (resources_count{instance=~\"$instance\",resource_type!~\"Dataplane|DataplaneInsight|Zone|ZoneInsight\"})
  2. sum by (zone, instance) (resources_count{instance=~\"$instance\",resource_type=\"Dataplane\"})

Am I right that only mesh and zone are global-scoped resources? If it's true then this implementation should be correct, because this metric is not used to present number of meshes or zones.

I am going to add a unit test and then I will change this draft to pull request.

@lobkovilya
Copy link
Contributor

In addition to zone and mesh there is also config. Anyway, we're going to add new resources both mesh-scoped and global-scoped and it'd great to have them in resources_count without additional changes.

So I'd suggest having this code in a more generic manner: iterate over all types that we have and use method Scope to decide where to take the value from - resManager for global-scope and MeshInsight for mesh-scope.

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>
@jewertow jewertow marked this pull request as ready for review January 15, 2021 20:34
@jewertow jewertow requested a review from a team as a code owner January 15, 2021 20:34
Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>
Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>
Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>
Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>
@jewertow
Copy link
Contributor Author

jewertow commented Jan 17, 2021

I added a test for resource counter. To make it possible I had to refactor existing code. I added new function Start to counter that accepts a ticker to enable injecting test tickers. The most important point is that the counter test relies on resyncer. Creating a resyncer adds a little bit of complexity to setup of this test, but simplifies logic of a test. What do you think?

Copy link
Contributor

@lobkovilya lobkovilya left a comment

Choose a reason for hiding this comment

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

Overall looks great, thanks for your contribution! Just a small comment

return s.start(stop, ticker)
}

func (s *storeCounter) start(stop <-chan struct{}, ticker *time.Ticker) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can rename this to StartWithTicker and call it from Start

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>
@lobkovilya lobkovilya merged commit 1e1eb76 into kumahq:master Jan 19, 2021
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.

Use MeshInsights for resource counter metric
3 participants