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

fix(service-insights): Show gateway services in service-insights #2711

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

lahabana
Copy link
Contributor

Summary

We were hidding gateway in service-insights.

This was unintuitive and also caused problems by not showing
up in the serviceMap in grafana

Full changelog

  • add gateway in service insights computation.

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Add backport-to-stable label if the code is backwards compatible. Otherwise, list breaking changes.

@lahabana lahabana requested a review from a team as a code owner August 31, 2021 11:20
We were hidding gateway in service-insights.

This was unintuitive and also caused problems by not showing
up in the serviceMap in grafana

Signed-off-by: Charly Molter <charly.molter@konghq.com>
@lahabana
Copy link
Contributor Author

It really feels like this resyncer could be simplified and end up being quicker and require less read/writes.

@lobkovilya
Copy link
Contributor

@lahabana that would be great, do you have anything particular in mind?

@codecov-commenter
Copy link

Codecov Report

Merging #2711 (2ab144f) into master (b636a9a) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2711      +/-   ##
==========================================
+ Coverage   52.20%   52.27%   +0.06%     
==========================================
  Files         865      865              
  Lines       49297    49299       +2     
==========================================
+ Hits        25737    25771      +34     
+ Misses      21478    21440      -38     
- Partials     2082     2088       +6     
Impacted Files Coverage Δ
pkg/insights/resyncer.go 67.16% <100.00%> (+0.99%) ⬆️
pkg/xds/generator/direct_access_proxy_generator.go 83.90% <0.00%> (+1.14%) ⬆️
pkg/core/resources/model/rest/resource.go 69.23% <0.00%> (+1.28%) ⬆️
pkg/plugins/resources/memory/store.go 82.06% <0.00%> (+4.34%) ⬆️
pkg/core/bootstrap/autoconfig.go 54.46% <0.00%> (+8.03%) ⬆️
.../core/managers/apis/ratelimit/ratelimit_manager.go 45.16% <0.00%> (+9.67%) ⬆️
pkg/core/resources/store/customizable_store.go 88.88% <0.00%> (+11.11%) ⬆️
pkg/insights/components.go 100.00% <0.00%> (+30.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b636a9a...2ab144f. Read the comment docs.

@lahabana
Copy link
Contributor Author

Yes the main thing is that the meshInsight never needs to get recomputed if the serviceInsights haven't changed. So we'd check if any of the serviceInsight change before computing the meshInsight.

We'd also wouldn't need to read the serviceInsight again when computing the meshInsight (as we've just computed them).
I'd also would quite like to avoid the goroutine in the Start as it seems to be unnecessary (Recv could simply return a channel).

@lobkovilya
Copy link
Contributor

Isn't it vice versa? MeshInsight also includes policies, so MeshInsight could change while ServiceInsight is the same

@lahabana lahabana merged commit 1ec22de into kumahq:master Aug 31, 2021
mergify bot pushed a commit that referenced this pull request Aug 31, 2021
We were hidding gateway in service-insights.

This was unintuitive and also caused problems by not showing
up in the serviceMap in grafana

Signed-off-by: Charly Molter <charly.molter@konghq.com>
(cherry picked from commit 1ec22de)
@lahabana
Copy link
Contributor Author

Yes you are right. Though I believe we'd still avoid some level a recomputation and reading things twice.

lahabana added a commit that referenced this pull request Sep 1, 2021
…) (#2712)

We were hidding gateway in service-insights.

This was unintuitive and also caused problems by not showing
up in the serviceMap in grafana

Signed-off-by: Charly Molter <charly.molter@konghq.com>
(cherry picked from commit 1ec22de)

Co-authored-by: Charly Molter <charly.molter@konghq.com>
nikita15p pushed a commit to nikita15p/kuma that referenced this pull request Sep 28, 2021
…ahq#2711)

We were hidding gateway in service-insights.

This was unintuitive and also caused problems by not showing
up in the serviceMap in grafana

Signed-off-by: Charly Molter <charly.molter@konghq.com>
nikita15p pushed a commit to nikita15p/kuma that referenced this pull request Sep 28, 2021
…ahq#2711)

We were hidding gateway in service-insights.

This was unintuitive and also caused problems by not showing
up in the serviceMap in grafana

Signed-off-by: Charly Molter <charly.molter@konghq.com>
@lahabana lahabana deleted the fix/showGateway branch March 29, 2024 12:42
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