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(telemetry) add Kong router flavor #4762

Merged
merged 2 commits into from
Oct 3, 2023
Merged

feat(telemetry) add Kong router flavor #4762

merged 2 commits into from
Oct 3, 2023

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Sep 29, 2023

What this PR does / why we need it:

Includes the router flavor in reports.

Special notes for your reviewer:

Tests normally can't run telemetry against a real instance, so an example with that flipped in KONG_TEST_EXPRESSION_ROUTES=true KONG_TEST_CLUSTER=kind:kind GOFLAGS="-tags=integration_tests" dlv test ./test/integration

(dlv) p cfg["router_flavor"]
interface {}(string) "traditional"
(dlv) n
> github.com/kong/kubernetes-ingress-controller/v2/internal/manager/telemetry.SetupAnonymousReports() ./internal/manager/telemetry/reports.go:82 (PC: 0x3e0a70a)
    77:		kongDB, ok := cfg["database"].(string)
    78:		if !ok {
    79:			return nil, fmt.Errorf("malformed database configuration found in Kong client root")
    80:		}
    81:		routerFlavor, ok := cfg["router_flavor"].(string)
=>  82:		if !ok {
    83:			// version to old to use router flavor, de facto traditional
    84:			routerFlavor = "traditional"
    85:		}
    86:	
    87:		fixedPayload := Payload{
(dlv) p ok
true
...
(dlv) p fixedPayload
github.com/kong/kubernetes-telemetry/pkg/types.ProviderReport [
	"v": "NOT_SET", 
	"kv": "3.3.1", 
	"db": "off", 
	"r": "traditional", 
	"id": github.com/google/uuid.UUID [100,208,215,11,47,138,78,62,175,54,171,192,93,253,54,188], 
]

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

@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (be62bf7) 77.8% compared to head (aaa811a) 77.6%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4762     +/-   ##
=======================================
- Coverage   77.8%   77.6%   -0.2%     
=======================================
  Files        163     163             
  Lines      18526   18532      +6     
=======================================
- Hits       14416   14399     -17     
- Misses      3305    3322     +17     
- Partials     805     811      +6     
Files Coverage Δ
internal/manager/telemetry/reports.go 47.6% <33.3%> (-1.5%) ⬇️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

czeslavo
czeslavo previously approved these changes Oct 2, 2023
Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

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

I think that's not exactly what Konnect expected from us (it was about the UI side requiring some way to tell if a route we populate in Konnect is expression or traditional and we concluded it can be easily derived from the route itself).

Probably having telemetry for this won't hurt us and can be useful in the future. Worth to note though that we already have telemetry for tracking the feature gates so the ExpressionsRoutes feature is kinda tracked even without this.

Remove a log that would emit during an expected test shutdown condition.
This suggested that condition was the problem, rather than other
problems earlier.
@rainest
Copy link
Contributor Author

rainest commented Oct 2, 2023

I expect we'll remove the feature flag (GA support for controller-managed expression routes) before the gateway removes traditional (if it does at all), so it'll be useful later.

Also removed a rather unhelpful log. Failures due to the (unfortunately very touchy) expected value not matching looked like:

     telemetry_test.go:82: 
        	Error Trace:	/home/runner/work/kubernetes-ingress-controller/kubernetes-ingress-controller/test/envtest/telemetry_test.go:82
        	Error:      	Condition never satisfied
        	Test:       	TestTelemetry
    telemetry_test.go:85: failed to remove stanza "id" from report: stanza "id=" not found in report: 

Between not having a message on the Eventually and empty reports apparently being normal (not sure why, but they do show up) at the end of the test, I was very confused searching for why the changes here made part of the static report "disappear".

If we break it again, we'll now see:

    telemetry_test.go:82: 
        	Error Trace:	/tmp/ingress-controller/test/envtest/telemetry_test.go:82
        	Error:      	Condition never satisfied
        	Test:       	TestTelemetry
        	Messages:   	telemetry report never matched expected value

@rainest rainest merged commit ebb2c52 into main Oct 3, 2023
32 checks passed
@rainest rainest deleted the feat/router-tele branch October 3, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants