api: Adding Zipkin Tracing support by alexandermarston · Pull Request #3630 · envoyproxy/gateway

alexandermarston · 2024-06-19T15:05:25Z

What type of PR is this?
API to support Zipkin provider for Tracing telemetry.

What this PR does / why we need it:
At present, Envoy Gateway only supports the OpenTelemetry tracing provider.

This PR updates the underlying APIs to also support the Zipkin provider.

In addition, it also exposes a new option to specify provider specific configuration such as TraceId128Bit and SharedSpanContext for Zipkin.

Which issue(s) this PR fixes:
Related to #1827

https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/trace/v3/zipkin.proto

Signed-off-by: Alex Marston <[email protected]>

api/v1alpha1/tracing_types.go

zirain · 2024-06-19T15:10:54Z

please revert changes under /internal

Signed-off-by: Alex Marston <[email protected]>

alexandermarston · 2024-06-19T15:29:56Z

please revert changes under /internal

@zirain I have done so - shall I update the site docs? (site/content/en/latest/api/extension_types.md)

api/v1alpha1/tracing_types.go

zirain · 2024-06-19T15:35:14Z

api/v1alpha1/tracing_types.go

+
+// ZipkinConfiguration defines optional configuration for the Zipkin tracing provider.
+type ZipkinConfiguration struct {
+	// TraceId_128Bit determines whether a 128bit trace id will be used


Suggested change

// TraceId_128Bit determines whether a 128bit trace id will be used

// TraceId128Bit determines whether a 128bit trace id will be used

@basvanbeek is it recommended to use 128bit by default?

The default value is false, which will result in a 64 bit trace id being used.

https://pkg.go.dev/github.com/envoyproxy/go-control-plane/envoy/config/trace/[email protected]#ZipkinConfig.TraceId_128Bit

Most libraries do 64bit as default allowing to be switched to 128 bit for those worried about trace collisions in very high request rate systems, or where retention is set to high.

Co-authored-by: zirain <[email protected]> Signed-off-by: Alex Marston <[email protected]>

zirain · 2024-06-19T15:36:19Z

api/v1alpha1/tracing_types.go

+	// share the same span context. Defaults to true.
+	// +kubebuilder:default=true
+	// +optional
+	SharedSpanContext bool `json:"sharedSpanContext,omitempty"`


should we rename this to DisableSharedSpanContext and make the default value to false?

Seeing as SharedSpanContext defaults to true in the backend anyway, I think it makes sense to keep the naming of this field consistent.

An API should imporve the UX of most of the end users, envoy may use specific values due to backward compatibility.

zirain · 2024-06-19T15:37:13Z

please revert changes under /internal

@zirain I have done so - shall I update the site docs? (site/content/en/latest/api/extension_types.md)

run make -k gen-check will auto generated the docs.

Signed-off-by: Alex Marston <[email protected]>

codecov · 2024-06-20T00:05:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.29%. Comparing base (9830c4d) to head (b906c34).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3630      +/-   ##
==========================================
- Coverage   68.29%   68.29%   -0.01%     
==========================================
  Files         170      170              
  Lines       20760    20760              
==========================================
- Hits        14179    14178       -1     
+ Misses       5563     5562       -1     
- Partials     1018     1020       +2

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

Signed-off-by: Alex Marston <[email protected]>

api/v1alpha1/tracing_types.go

Signed-off-by: Alex Marston <[email protected]>

zirain · 2024-06-20T08:36:32Z

api/v1alpha1/tracing_types.go

+	// CollectorHostname defines the hostname to use when sending spans
+	// to the Zipkin collector endpoint.
+	// +optional
+	CollectorHostname *string `json:"collectorHostname,omitempty"`


Is this necessary for first iterate? There's a discussion about where's the right place for authoriy, here or under BackendRef

It's not necessary but I lacked the context on the other discussion. Happy to pull this out and wait for the outcome of that discussion

The standard Zipkin ecosystem does nothing with this.

Signed-off-by: Alex Marston <[email protected]>

zirain

LGTM, @basvanbeek can you take a look?

basvanbeek

LGTM

arkodg · 2024-06-20T19:39:49Z

api/v1alpha1/tracing_types.go

+	// when creating a new trace instance. If set to false, a 64bit trace
+	// id will be used.
+	// +optional
+	TraceID128Bit *bool `json:"traceId128Bit,omitempty"`


I understand this is creating a 1:1 mapping with envoy https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/trace/v3/zipkin.proto#extension-envoy-tracers-zipkin but this name is confusing
prefer if this was TraceIDSize/TraceIDLength *TraceIDType which could default to 64

Would EnableTraceID128Bit or Enable128BitTraceID be a better name?

yeah I'm a +1 for Enable128BitTraceID wdyt @envoyproxy/gateway-maintainers @basvanbeek

Enable128BitTraceID has my preference too.

Thank you for input. Field name updated

Signed-off-by: Alex Marston <[email protected]>

arkodg

LGTM thanks !

zirain · 2024-06-24T16:20:14Z

/retest

* api: Adding Zipkin Tracing support Signed-off-by: Alex Marston <[email protected]> * rollback /internal changes Signed-off-by: Alex Marston <[email protected]> * nest zipkin configuration under TracingProvider struct Signed-off-by: Alex Marston <[email protected]> * rollback internal testdata changes Signed-off-by: Alex Marston <[email protected]> * rollback site changes Signed-off-by: Alex Marston <[email protected]> * Change ZipkinConfiguration to a pointer. Co-authored-by: zirain <[email protected]> Signed-off-by: Alex Marston <[email protected]> * update deepcopy Signed-off-by: Alex Marston <[email protected]> * update description for TraceId128Bit field Signed-off-by: Alex Marston <[email protected]> * update site docs Signed-off-by: Alex Marston <[email protected]> * rename Zipkin config struct Signed-off-by: Alex Marston <[email protected]> * update api Signed-off-by: Alex Marston <[email protected]> * pointers for optional fields Signed-off-by: Alex Marston <[email protected]> * remove CollectorHostname Signed-off-by: Alex Marston <[email protected]> * TraceID128Bit -> Enable128BitTraceID Signed-off-by: Alex Marston <[email protected]> --------- Signed-off-by: Alex Marston <[email protected]> Signed-off-by: Alex Marston <[email protected]> Co-authored-by: zirain <[email protected]> Signed-off-by: bjlhlin <[email protected]>

alexandermarston requested a review from a team as a code owner June 19, 2024 15:05

api: Adding Zipkin Tracing support

77f0415

Signed-off-by: Alex Marston <[email protected]>

alexandermarston force-pushed the telemetry-tracing-zipkin-api branch from 0c55a37 to 77f0415 Compare June 19, 2024 15:06

zirain reviewed Jun 19, 2024

View reviewed changes

api/v1alpha1/tracing_types.go Outdated Show resolved Hide resolved

alexandermarston added 4 commits June 19, 2024 16:19

rollback /internal changes

927b2c0

Signed-off-by: Alex Marston <[email protected]>

nest zipkin configuration under TracingProvider struct

b96872c

Signed-off-by: Alex Marston <[email protected]>

rollback internal testdata changes

3db1a57

Signed-off-by: Alex Marston <[email protected]>

rollback site changes

95b7f29

Signed-off-by: Alex Marston <[email protected]>

zirain reviewed Jun 19, 2024

View reviewed changes

api/v1alpha1/tracing_types.go Outdated Show resolved Hide resolved

zirain reviewed Jun 19, 2024

View reviewed changes

Change ZipkinConfiguration to a pointer.

4968c72

Co-authored-by: zirain <[email protected]> Signed-off-by: Alex Marston <[email protected]>

zirain reviewed Jun 19, 2024

View reviewed changes

alexandermarston added 4 commits June 19, 2024 16:46

update deepcopy

53704b1

Signed-off-by: Alex Marston <[email protected]>

update description for TraceId128Bit field

4d88f89

Signed-off-by: Alex Marston <[email protected]>

update site docs

4aff10c

Signed-off-by: Alex Marston <[email protected]>

rename Zipkin config struct

a9d540a

Signed-off-by: Alex Marston <[email protected]>

alexandermarston and others added 2 commits June 20, 2024 08:02

Merge branch 'main' into telemetry-tracing-zipkin-api

4cc1cae

update api

22cfadf

Signed-off-by: Alex Marston <[email protected]>

zirain reviewed Jun 20, 2024

View reviewed changes

api/v1alpha1/tracing_types.go Outdated Show resolved Hide resolved

zirain reviewed Jun 20, 2024

View reviewed changes

api/v1alpha1/tracing_types.go Outdated Show resolved Hide resolved

pointers for optional fields

9a7c0c3

Signed-off-by: Alex Marston <[email protected]>

zirain reviewed Jun 20, 2024

View reviewed changes

remove CollectorHostname

f380b41

Signed-off-by: Alex Marston <[email protected]>

zirain previously approved these changes Jun 20, 2024

View reviewed changes

basvanbeek approved these changes Jun 20, 2024

View reviewed changes

arkodg reviewed Jun 20, 2024

View reviewed changes

TraceID128Bit -> Enable128BitTraceID

9c85a03

Signed-off-by: Alex Marston <[email protected]>

alexandermarston dismissed zirain’s stale review via 9c85a03 June 24, 2024 15:24

Merge branch 'main' into telemetry-tracing-zipkin-api

b906c34

alexandermarston requested review from arkodg, basvanbeek and zirain June 24, 2024 15:25

arkodg approved these changes Jun 24, 2024

View reviewed changes

basvanbeek approved these changes Jun 24, 2024

View reviewed changes

zirain approved these changes Jun 24, 2024

View reviewed changes

zirain merged commit 51196b4 into envoyproxy:main Jun 24, 2024

alexandermarston deleted the telemetry-tracing-zipkin-api branch June 24, 2024 16:52

alexandermarston mentioned this pull request Jun 24, 2024

feat: implement zipkin tracing #3668

Merged

	// TraceId_128Bit determines whether a 128bit trace id will be used
	// TraceId128Bit determines whether a 128bit trace id will be used

Conversation

alexandermarston commented Jun 19, 2024

Uh oh!

Uh oh!

zirain commented Jun 19, 2024

Uh oh!

alexandermarston commented Jun 19, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zirain commented Jun 19, 2024

Uh oh!

codecov bot commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zirain left a comment

Choose a reason for hiding this comment

Uh oh!

basvanbeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

zirain commented Jun 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jun 20, 2024 •

edited

Loading