-
Notifications
You must be signed in to change notification settings - Fork 329
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far! I had some minor thoughts on code structuring and maybe using an interface, but most of the feedback is around capitalizing names in the docs like OpenConcensus
and gRPC
, etc
internal/telemetry/telemetry.go
Outdated
EnableOpenCensusExporter bool | ||
OpenCensusExporterOptions []ocagent.ExporterOption | ||
|
||
EnableDatadogExporter bool | ||
DatadogExporterOptions datadog.Options | ||
|
||
EnableZpages bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we anticipate many more telemetry providers? If so, I feel like the Enable<name> bool
here and the if t.Enabled<name>
above seem like it could be placed behind an interface, and each implementation would be its own file like telemetry/datadog.go
or so. Then when parsing the flags maybe WithDatadogExporter
and the like actually append a type from those implementations, and the telemetry.Run
code just uses the common interface methods for setup and tear-down functions.
Just a thought for consideration and discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elaborating / clarifying:
Instead of these bools and options, it would be more like
// internal/telemetry/telemetry.go
type Exporter Interface {
Options() ocagent.ExporterOption // returns the options
Closer() func() // returns function that closes things down
}
type telemetry struct {
[...]
Exporters []*Exporter
[...]
}
func Run(opts ...Option) error {
[...]
var t telemetry
for _, opt := range opts {
opt(&t)
}
for _, e := range t.Exporters {
exporter, err := ocagent.NewExporter(e.Options())
if err != nil {
return status.Errorf(codes.InvalidArgument, "failed to initalize exporter: %s", err)
}
octrace.RegisterExporter(exporter)
ocview.RegisterExporter(exporter)
closeFuncs = append(closeFuncs, e.Closer())
}
Again, just thought/discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments to start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things to go along with what has already been said.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me! Most of my comments/review suggestions are just making sure we stay consistent when we refer to different tools in logs or doc strings. Feel free to apply those in a big commit since they're all related!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more capitalizations and doc nits! Excellent work on documentation overall, this is great.
Co-authored-by: Rae Krantz <[email protected]>
Co-authored-by: Rae Krantz <[email protected]>
Co-authored-by: Rae Krantz <[email protected]>
Co-authored-by: Rae Krantz <[email protected]>
Co-authored-by: Rae Krantz <[email protected]>
Co-authored-by: Rae Krantz <[email protected]>
Co-authored-by: Rae Krantz <[email protected]>
Co-authored-by: Rae Krantz <[email protected]>
This PR enables server telemetry, which creates OpenCensus traces and timing/count stats for each grpc request.
Closes #2364
Traces in datadog:
Traces in jeager:

Stats in datadog:
What changed
telemetry
package, responsible for configuring opencensus and its exporterswaypoint server run
for selecting and configuring either an opencensus agent exporter, or a direct datadog exportertelemetry
top-level directory that contains a docker-compose with example infra (an oc-agent, an oc-collector, a datadog agent, and jeager), and an example of how to use it.How to verify
Check out the
telemetry
top-level directory, and try running the example!