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(tracing): Support for OpenTelemetry #860

Merged
merged 14 commits into from
Nov 30, 2022

Conversation

aniaan
Copy link
Collaborator

@aniaan aniaan commented Nov 28, 2022

  1. Use OpenTelemetry to replace Zipkin
  2. Support a variety of Exporter, Zipkin, Jagger, OTLP
  3. Support trace-context and b3 context propagation formats. https://opentelemetry.io/docs/instrumentation/go/manual/#propagators-and-context
  4. tracing.tags is replaced by tracing.attributes, in order to be consistent with the OpenTelemetry concept

@aniaan aniaan requested a review from localvar November 28, 2022 16:55
@localvar localvar requested a review from suchen-sci November 29, 2022 00:41
@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2022

Codecov Report

Base: 77.04% // Head: 76.05% // Decreases project coverage by -0.99% ⚠️

Coverage data is based on head (698419e) compared to base (b80a9cf).
Patch coverage: 42.81% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #860      +/-   ##
==========================================
- Coverage   77.04%   76.05%   -1.00%     
==========================================
  Files         108      110       +2     
  Lines       12340    12698     +358     
==========================================
+ Hits         9507     9657     +150     
- Misses       2300     2494     +194     
- Partials      533      547      +14     
Impacted Files Coverage Δ
pkg/cluster/cluster.go 60.10% <ø> (ø)
pkg/tracing/tracing.go 28.17% <27.15%> (ø)
pkg/filters/opafilter/opafilter.go 75.00% <75.00%> (ø)
pkg/filters/proxy/pool.go 79.30% <100.00%> (-0.88%) ⬇️
pkg/object/httpserver/mux.go 79.41% <100.00%> (+0.15%) ⬆️
pkg/filters/headerlookup/headerlookup.go 86.01% <0.00%> (+1.39%) ⬆️

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.

Tags map[string]string `json:"tags" jsonschema:"omitempty"`
Zipkin *ZipkinSpec `json:"zipkin" jsonschema:"required"`
ServiceName string `json:"serviceName" jsonschema:"required,minLength=1"`
Attributes map[string]string `json:"attributes" jsonschema:"omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

better keep both Tags and Attributes, and ignore Tags if Attributes is not empty, this result in better backward compatibility.


// Validate validates Spec.
func (spec *JaegerSpec) Validate() error {

Copy link
Collaborator

Choose a reason for hiding this comment

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

per coding style, please remove these blank lines, and the blank lines before the closing } of a function.

Comment on lines 329 to 330
return resource.Merge(resource.Default(),
resource.NewWithAttributes(semconv.SchemaURL, attrs...))
Copy link
Collaborator

Choose a reason for hiding this comment

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

per https://github.com/golang/go/wiki/CodeReviewComments#line-length
propose not breaking a lengthy statement into several lines.

if it is too long, try breaking it into several statements, this makes the code more readable.

there are other cases.

Comment on lines 340 to 346
if sampleRate <= 0 {
return sdktrace.NeverSample()
} else if sampleRate >= 1 {
return sdktrace.AlwaysSample()
} else {
return sdktrace.TraceIDRatioBased(sampleRate)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this makes the code more readable.

Suggested change
if sampleRate <= 0 {
return sdktrace.NeverSample()
} else if sampleRate >= 1 {
return sdktrace.AlwaysSample()
} else {
return sdktrace.TraceIDRatioBased(sampleRate)
}
if sampleRate <= 0 {
return sdktrace.NeverSample()
}
if sampleRate >= 1 {
return sdktrace.AlwaysSample()
}
return sdktrace.TraceIDRatioBased(sampleRate)

Comment on lines 374 to 381
} else {
if exp, err := spec.Zipkin.newExporter(); err == nil {
exporters = []sdktrace.SpanExporter{exp}
} else {
return nil, err
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} else {
if exp, err := spec.Zipkin.newExporter(); err == nil {
exporters = []sdktrace.SpanExporter{exp}
} else {
return nil, err
}
}
} else if exp, err := spec.Zipkin.newExporter(); err == nil {
exporters = []sdktrace.SpanExporter{exp}
} else {
return nil, err
}

@suchen-sci suchen-sci merged commit f440e84 into easegress-io:main Nov 30, 2022
caoshengdong pushed a commit to caoshengdong/easegress that referenced this pull request Dec 20, 2022
* feat(tracing): init

* fix(tracing): add propagator

* fix(mod): upgrade

* fix(test): add test

* docs(controllers.md): update tracing spec

* fix(doc): fix lint

* docs(tracing.md): update

* fix(tracing): Support for multiple exporter configurations

* fix(tracing): add logger

* fix(license): add license

* fix(tracing): Backward compatible

* fix(tracing): fix null pointer

* fix(tracing): add tags && update doc
@aniaan aniaan deleted the feat/opentelemetry branch February 10, 2023 08:59
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.

support different types of tracer by configuration, such as skywalking, currently only zipkin
4 participants