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

refactor: use kubernetes-telemetry package #3552

Merged
merged 6 commits into from
Feb 16, 2023
Merged

refactor: use kubernetes-telemetry package #3552

merged 6 commits into from
Feb 16, 2023

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Feb 14, 2023

What this PR does / why we need it:

This introduces usage of https://github.com/Kong/kubernetes-telemetry/ in KIC.

The existing code for anonymous reports was left as is in this PR but we can remove it if we decide that's the approach we'd like to take.

Reports have been checked to produce the same data (and more) as it can be observed in the added UTs.

Which issue this PR fixes:

Fixes #3541

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@pmalek pmalek added this to the KIC v2.9.0 milestone Feb 14, 2023
@pmalek pmalek self-assigned this Feb 14, 2023
@pmalek pmalek marked this pull request as ready for review February 14, 2023 18:38
@pmalek pmalek requested a review from a team as a code owner February 14, 2023 18:38
rainest
rainest previously approved these changes Feb 15, 2023
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Added a make generate commit to make the linter happy. Hopefully whatever CI license issue broke the other test earlier is gone now and this just auto-merges. If not, such is life.

@rainest rainest enabled auto-merge (rebase) February 15, 2023 00:54
@rainest rainest self-requested a review February 15, 2023 01:01
@rainest
Copy link
Contributor

rainest commented Feb 15, 2023

Mmm, maybe not quite. Looks like we're stuck on actually using 0.6.1 because someone decided to start using new Go features? See failures in #3524.

This otherwise looks fine, but I think we want to fix that first if we're going to start using the 0.6.1 CRDs. I suspect the 0.6.1 CRDs alone wouldn't actually cause issues, but I think we probably want the module also for consistency.

Edit: looks like it also breaks conformance. I was wrong!

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Base: 72.1% // Head: 72.0% // Decreases project coverage by -0.2% ⚠️

Coverage data is based on head (62d05ba) compared to base (28dc3cb).
Patch coverage: 25.9% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #3552     +/-   ##
=======================================
- Coverage   72.1%   72.0%   -0.2%     
=======================================
  Files        127     128      +1     
  Lines      14771   14843     +72     
=======================================
+ Hits       10657   10694     +37     
- Misses      3437    3460     +23     
- Partials     677     689     +12     
Impacted Files Coverage Δ
internal/manager/run.go 42.7% <0.0%> (ø)
internal/manager/telemetry/reports.go 0.0% <0.0%> (ø)
internal/manager/telemetry/manager.go 40.0% <40.0%> (ø)
internal/util/k8s.go 92.5% <100.0%> (ø)
...nternal/controllers/gateway/tlsroute_controller.go 69.3% <0.0%> (-4.1%) ⬇️
...nternal/controllers/gateway/udproute_controller.go 71.6% <0.0%> (ø)
internal/manager/utils/reports.go
internal/dataplane/parser/parser.go 92.4% <0.0%> (+0.8%) ⬆️
internal/dataplane/kongstate/service.go 67.3% <0.0%> (+1.2%) ⬆️
...nternal/controllers/gateway/tcproute_controller.go 72.0% <0.0%> (+2.0%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pmalek
Copy link
Member Author

pmalek commented Feb 15, 2023

It seems we needed an exclude directive, not a replace. All should be good now.

@pmalek pmalek requested a review from a team February 15, 2023 08:34
go.mod Outdated Show resolved Hide resolved
internal/manager/telemetry/manager.go Outdated Show resolved Hide resolved
internal/manager/telemetry/manager.go Outdated Show resolved Hide resolved
internal/manager/telemetry/manager_test.go Outdated Show resolved Hide resolved
@rainest rainest merged commit 9ffebae into main Feb 16, 2023
@rainest rainest deleted the telemetry-pkg branch February 16, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use kubernetes-telemetry instead of custom code in KIC's tree
3 participants