Skip to content

api: Adding Zipkin Tracing support #3630

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Jun 24, 2024
Merged

api: Adding Zipkin Tracing support #3630

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
77f0415
api: Adding Zipkin Tracing support
alexandermarston Jun 19, 2024
927b2c0
rollback /internal changes
alexandermarston Jun 19, 2024
b96872c
nest zipkin configuration under TracingProvider struct
alexandermarston Jun 19, 2024
3db1a57
rollback internal testdata changes
alexandermarston Jun 19, 2024
95b7f29
rollback site changes
alexandermarston Jun 19, 2024
4968c72
Change ZipkinConfiguration to a pointer.
alexandermarston Jun 19, 2024
53704b1
update deepcopy
alexandermarston Jun 19, 2024
4d88f89
update description for TraceId128Bit field
alexandermarston Jun 19, 2024
4aff10c
update site docs
alexandermarston Jun 19, 2024
a9d540a
rename Zipkin config struct
alexandermarston Jun 19, 2024
4cc1cae
Merge branch 'main' into telemetry-tracing-zipkin-api
alexandermarston Jun 20, 2024
22cfadf
update api
alexandermarston Jun 20, 2024
9a7c0c3
pointers for optional fields
alexandermarston Jun 20, 2024
f380b41
remove CollectorHostname
alexandermarston Jun 20, 2024
9c85a03
TraceID128Bit -> Enable128BitTraceID
alexandermarston Jun 24, 2024
b906c34
Merge branch 'main' into telemetry-tracing-zipkin-api
alexandermarston Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions api/v1alpha1/tracing_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,22 @@ type ProxyTracing struct {
// If provider is kubernetes, pod name and namespace are added by default.
CustomTags map[string]CustomTag `json:"customTags,omitempty"`
// Provider defines the tracing provider.
// Only OpenTelemetry is supported currently.
Provider TracingProvider `json:"provider"`
}

type TracingProviderType string

const (
TracingProviderTypeOpenTelemetry TracingProviderType = "OpenTelemetry"
TracingProviderTypeZipkin TracingProviderType = "Zipkin"
)

// TracingProvider defines the tracing provider configuration.
//
// +kubebuilder:validation:XValidation:message="host or backendRefs needs to be set",rule="has(self.host) || self.backendRefs.size() > 0"
type TracingProvider struct {
// Type defines the tracing provider type.
// EG currently only supports OpenTelemetry.
// +kubebuilder:validation:Enum=OpenTelemetry
// +kubebuilder:validation:Enum=OpenTelemetry;Zipkin
// +kubebuilder:default=OpenTelemetry
Type TracingProviderType `json:"type"`
// Host define the provider service hostname.
Expand All @@ -58,6 +57,10 @@ type TracingProvider struct {
// +kubebuilder:validation:XValidation:message="only support Service kind.",rule="self.all(f, f.kind == 'Service')"
// +kubebuilder:validation:XValidation:message="BackendRefs only supports Core group.",rule="self.all(f, f.group == '')"
BackendRefs []BackendRef `json:"backendRefs,omitempty"`
// Zipkin defines optional configuration for the Zipkin tracing provider.
// +optional
// +kubebuilder:validation:XValidation:message="only supports Zipkin provider type.",rule="self.type == 'Zipkin'"
Zipkin ZipkinConfiguration `json:"zipkin,omitempty"`
}

type CustomTagType string
Expand Down Expand Up @@ -114,3 +117,16 @@ type RequestHeaderCustomTag struct {
// +optional
DefaultValue *string `json:"defaultValue,omitempty"`
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

// when creating a new trace instance.
// +optional
TraceId128Bit bool `json:"traceId128Bit,omitempty"`
// SharedSpanContext determines whether client and server spans will
// share the same span context. Defaults to true.
// +kubebuilder:default=true
// +optional
SharedSpanContext bool `json:"sharedSpanContext,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

}
16 changes: 16 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 22 additions & 6 deletions charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoyproxies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10864,9 +10864,7 @@ spec:
If provider is kubernetes, pod name and namespace are added by default.
type: object
provider:
description: |-
Provider defines the tracing provider.
Only OpenTelemetry is supported currently.
description: Provider defines the tracing provider.
properties:
backendRefs:
description: |-
Expand Down Expand Up @@ -10972,12 +10970,30 @@ spec:
type: integer
type:
default: OpenTelemetry
description: |-
Type defines the tracing provider type.
EG currently only supports OpenTelemetry.
description: Type defines the tracing provider type.
enum:
- OpenTelemetry
- Zipkin
type: string
zipkin:
description: Zipkin defines optional configuration for
the Zipkin tracing provider.
properties:
sharedSpanContext:
default: true
description: |-
SharedSpanContext determines whether client and server spans will
share the same span context. Defaults to true.
type: boolean
traceId128Bit:
description: |-
TraceId_128Bit determines whether a 128bit trace id will be used
when creating a new trace instance.
type: boolean
type: object
x-kubernetes-validations:
- message: only supports Zipkin provider type.
rule: self.type == 'Zipkin'
required:
- type
type: object
Expand Down