-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
examples/features/opentelemetry: Include OpenTelemetry tracing plugin usage and output in example server and client #8056
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8056 +/- ##
==========================================
+ Coverage 82.16% 82.25% +0.09%
==========================================
Files 387 392 +5
Lines 38941 39140 +199
==========================================
+ Hits 31997 32196 +199
+ Misses 5619 5612 -7
- Partials 1325 1332 +7 🚀 New features to boost your workflow:
|
@@ -33,37 +33,90 @@ import ( | |||
"google.golang.org/grpc/stats/opentelemetry" | |||
|
|||
"github.com/prometheus/client_golang/prometheus/promhttp" | |||
"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" |
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.
why do we need otelgrpc?
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.
this will automatically instrument grpc client calls with otel tracing and metrics.
log.Fatalf("Error setting up tracing: %v", err) | ||
} | ||
defer func() { _ = tp.Shutdown(context.Background()) }() | ||
|
||
do := opentelemetry.DialOption(opentelemetry.Options{MetricsOptions: opentelemetry.MetricsOptions{MeterProvider: provider}}) |
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.
how wll the tracing work? You are not adding the trace options. Please see this POC code https://github.com/purnesh42H/grpc-go/pull/6/files#diff-719b01b6393a1e9b5a7d2a3908effb85dd904e5e2799f2ace99fab70d606f810R68
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.
done
|
||
// Make an RPC every second. This should trigger telemetry to be emitted from | ||
// the client and the server. | ||
for { | ||
r, err := c.UnaryEcho(ctx, &echo.EchoRequest{Message: "this is examples/opentelemetry"}) | ||
reqCtx, cancel := context.WithTimeout(ctx, time.Second) | ||
_, span := tracer.Start(reqCtx, "UnaryEchoClientCall") |
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.
we are showing the example of grpc tracing plugin. We don't need to start the span here outside. The example needs to show that once you add the trace options to client, grpc will create the required spans and traces and emit into exporter.
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.
done
if err != nil { | ||
log.Fatalf("Error setting up tracing: %v", err) | ||
} | ||
defer func() { _ = tp.Shutdown(context.Background()) }() | ||
|
||
so := opentelemetry.ServerOption(opentelemetry.Options{MetricsOptions: opentelemetry.MetricsOptions{MeterProvider: provider}}) |
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.
same comments as client. Please do similar to whats done here https://github.com/purnesh42H/grpc-go/pull/6/files#diff-719b01b6393a1e9b5a7d2a3908effb85dd904e5e2799f2ace99fab70d606f810R68
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.
done
f463f2b
to
58451b6
Compare
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.
@janardhanvissa please use any of the trace exporter from here https://opentelemetry.io/docs/languages/go/exporters/. May be otlptracehttp or stdouttrace are good candidates
Also, please refer to https://opentelemetry.io/docs/languages/go/getting-started/ for writing otel trace exporting parts in client and server. We should try to avoid as much custom code as possible here to make it easy and fast for users to pick up.
"google.golang.org/grpc" | ||
"google.golang.org/grpc/credentials/insecure" | ||
"google.golang.org/grpc/examples/features/proto/echo" | ||
experimental "google.golang.org/grpc/experimental/opentelemetry" |
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.
we can name it as oteltracing and "google.golang.org/grpc/stats/opentelemetry"
as otelmetrics
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.
done
"go.opentelemetry.io/otel/propagation" | ||
"go.opentelemetry.io/otel/sdk/metric" | ||
"go.opentelemetry.io/otel/sdk/trace" | ||
"go.opentelemetry.io/otel/sdk/trace/tracetest" |
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.
we shouldn't be using tracetest for example. Can you check other exporters that otel provides? For example, here in https://opentelemetry.io/docs/languages/go/getting-started/, they are using stdouttrace. We can use the same.
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.
using otlptracer
@@ -48,26 +51,49 @@ func main() { | |||
log.Fatalf("Failed to start prometheus exporter: %v", err) | |||
} | |||
provider := metric.NewMeterProvider(metric.WithReader(exporter)) | |||
go http.ListenAndServe(*prometheusEndpoint, promhttp.Handler()) | |||
|
|||
spanExporter := tracetest.NewInMemoryExporter() |
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.
As above, we shouldn't be using tracetest for examples. They are meant for testing only.
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.
done
spanExporter := tracetest.NewInMemoryExporter() | ||
spanProcessor := trace.NewSimpleSpanProcessor(spanExporter) | ||
tracerProvider := trace.NewTracerProvider(trace.WithSpanProcessor(spanProcessor)) | ||
textMapPropagator := propagation.NewCompositeTextMapPropagator(opentelemetry.GRPCTraceBinPropagator{}) |
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.
For main example, we should be using only W3CContextPropagator
. We will have separate example for opencensus migration, where we can showcase the GRPCTraceBinPropagator
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.
done
"go.opentelemetry.io/otel/sdk/metric" | ||
"go.opentelemetry.io/otel/sdk/trace" | ||
"go.opentelemetry.io/otel/sdk/trace/tracetest" |
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.
same comment as client about tracetest
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.
done
@@ -48,26 +51,49 @@ func main() { | |||
log.Fatalf("Failed to start prometheus exporter: %v", err) | |||
} | |||
provider := metric.NewMeterProvider(metric.WithReader(exporter)) | |||
go http.ListenAndServe(*prometheusEndpoint, promhttp.Handler()) | |||
|
|||
spanExporter := tracetest.NewInMemoryExporter() |
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.
let's rename the exporters more explicit. For trace, call it traceExporter and for metric call it metricExporter
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.
done
|
||
so := opentelemetry.ServerOption(opentelemetry.Options{MetricsOptions: opentelemetry.MetricsOptions{MeterProvider: provider}}) | ||
spanExporter := tracetest.NewInMemoryExporter() |
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.
same comment as client about naming of exporters.
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.
done
} | ||
|
||
go func() { | ||
log.Printf("Starting Prometheus metrics server at %s\n", *prometheusEndpoint) |
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.
same comment as client about prometheus
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.
done
} | ||
}() | ||
|
||
so := opentelemetry.ServerOption(opentelemetry.Options{ |
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.
same comment as client
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.
done
) | ||
|
||
var ( | ||
addr = flag.String("addr", ":50051", "the server address to connect to") | ||
prometheusEndpoint = flag.String("prometheus_endpoint", ":9465", "the Prometheus exporter endpoint") | ||
) | ||
|
||
// initTracer initializes OpenTelemetry tracing | ||
func initTracer() (*trace.TracerProvider, error) { | ||
exporter, err := stdouttrace.New(stdouttrace.WithPrettyPrint()) |
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.
@purnesh42H Can we use this way? Previously I did like this.
@@ -47,26 +47,33 @@ import ( | |||
var ( | |||
addr = flag.String("addr", ":50051", "the server address to connect to") | |||
prometheusEndpoint = flag.String("prometheus_endpoint", ":9465", "the Prometheus exporter endpoint") | |||
otlpEndpoint = flag.String("otlp_endpoint", "localhost:4318", "the OTLP collector endpoint") |
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.
nit: localhost is not needed. Keep it same as other endpoints. Also, lets be more explicit in naming since we both traces and metrics. the Prometheus exporter endpoint for metrics
and the OTLP collector endpoint for traces
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.
done
@@ -46,6 +46,8 @@ import ( | |||
var ( | |||
addr = flag.String("addr", ":50051", "the server address to connect to") | |||
prometheusEndpoint = flag.String("prometheus_endpoint", ":9464", "the Prometheus exporter endpoint") | |||
otlpEndpoint = flag.String("otlp_endpoint", "localhost:4318", "the OTLP collector endpoint") |
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.
same. localhost not needed
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.
Also, lets be more explicit in naming since we both traces and metrics. the Prometheus exporter endpoint for metrics
and the OTLP collector endpoint for traces
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.
done
@@ -41,6 +45,7 @@ require ( | |||
github.com/aws/aws-sdk-go-v2/service/sts v1.33.5 // indirect | |||
github.com/aws/smithy-go v1.22.1 // indirect | |||
github.com/beorn7/perks v1.0.1 // indirect | |||
github.com/cenkalti/backoff/v4 v4.3.0 // indirect |
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.
Why do we need this? Let's remove unnecessary dependencies.
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.
these are generated using go mod tidy and no other changes in my local.
@@ -54,6 +59,7 @@ require ( | |||
github.com/google/uuid v1.6.0 // indirect | |||
github.com/googleapis/enterprise-certificate-proxy v0.3.4 // indirect | |||
github.com/googleapis/gax-go/v2 v2.14.1 // indirect | |||
github.com/grpc-ecosystem/grpc-gateway/v2 v2.25.1 // indirect |
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.
Similar comment about its necessity. Let's remove unnecessary dependencies.
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.
these are generated using go mod tidy and no other changes in my local.
* **Docker Jaeger:** | ||
|
||
``` | ||
docker run -d -p 16686:16686 -p 14268:14268 \ |
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.
so, we need to have docker to run this example?
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.
we can do with or without docker
file: | ||
|
||
``` | ||
docker run -d -p 4317:4317 -p 4318:4318 \ |
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.
same: can we do it without docker command?
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.
yes we can do that. we are just providing the user to do either of the ways.
examples/go.mod
Outdated
@@ -1,11 +1,22 @@ | |||
module google.golang.org/grpc/examples | |||
|
|||
// NOTE: |
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.
what is this NOTE for? It is generated?
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.
It's not generated. Removing from here and keeping in the readme file
The client continuously makes RPC's to a server. The client and server both | ||
expose a prometheus exporter to listen and provide metrics. This defaults to | ||
:9464 for the server and :9465 for the client. | ||
* **Continuous RPC Calls:** The client continuously makes RPC calls to |
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.
why this change about metrics? Let's not make too many changes for metrics. Let's keep this PR focused on adding content for tracing only.
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.
done
|
||
* **Local Collector:** | ||
* Install the `otelcol-contrib` binary. | ||
* Create a configuration file (e.g., `collector-config.yaml`) |
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.
as i mentioned in the meeting. We should not ask the user to create a config.yaml file. We should provide a yaml which should work as is. If user want to modify that, they can
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.
done
Curling to the exposed Prometheus ports outputs the metrics recorded on the | ||
client and server. | ||
* Open `http://localhost:16686` in your browser. | ||
* Search for traces to visualize the RPC call flow. The traces will |
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.
you can mention the name of some of the traces that gRPC emit
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.
done
|
||
* This starts the gRPC client, which continuously makes RPC calls to the | ||
server and is configured to send trace data via OTLP and expose | ||
Prometheus metrics. |
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.
why "expose Prometheus metrics"? Aren't we talking about traces?
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.
done
* For the OpenTelemetry Collector Core distribution specifically, see | ||
[OpenTelemetry Collector Core Releases](https://github.com/open-telemetry/ | ||
opentelemetry-collector-releases/tree/main/distributions/otelcol). | ||
* Create a `collector-config.yaml` file with the following content: |
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.
we should have the yaml file in the example itself. Not mention in readme. In readme we should just mention the collector will use the this yaml.
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.
done
otlptracehttp.WithEndpoint("localhost:4318"), | ||
otlptracehttp.WithInsecure(), | ||
) | ||
traceExporter, err := otlptrace.New(context.Background(), otlpclient) |
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.
where did you change this? Its still creating a new context. Please use single context throughout which is cancelable
} | ||
defer cc.Close() | ||
c := echo.NewEchoClient(cc) | ||
|
||
// Make an RPC every second. This should trigger telemetry to be emitted from | ||
// Make an RPC every second. This should trigger telemetry on prometheus | ||
// server along with traces in the otlptracer exporter to be emitted from | ||
// the client and the server. |
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.
this is only about client, so let's omit server?
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.
done
otlptracehttp.WithEndpoint("localhost:4318"), | ||
otlptracehttp.WithInsecure(), | ||
) | ||
traceExporter, err := otlptrace.New(context.Background(), client) |
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.
Its not done. Please use same context throughout that was created initially
) | ||
|
||
var ( | ||
addr = flag.String("addr", ":50051", "the server address to connect to") | ||
prometheusEndpoint = flag.String("prometheus_endpoint", ":9464", "the Prometheus exporter endpoint") | ||
prometheusEndpoint = flag.String("prometheus_endpoint", ":9464", "the Prometheus exporter endpoint for metrics") | ||
otlpEndpoint = flag.String("otlp_endpoint", ":4318", "the OTLP collector endpoint for traces") |
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.
should be a different endpoint that client
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.
done
@@ -0,0 +1,20 @@ | |||
receivers: |
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.
please do separate configs for client and server
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.
done
…e collector config
This reverts commit c5a1dcf.
@@ -0,0 +1,19 @@ | |||
receivers: |
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.
same. Link the url for example collector config
} | ||
spanProcessor := trace.NewSimpleSpanProcessor(traceExporter) | ||
tp := trace.NewTracerProvider(trace.WithSpanProcessor(spanProcessor), trace.WithResource(res)) | ||
otel.SetTracerProvider(tp) |
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.
same. We shouldn't be doing it. gRPC tracing code is already doing it.
|
||
``` | ||
curl localhost:9464/metrics |
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.
We can't remove these because they are for metrics.
Create separate sections "See metrics" and "See traces". "See metrics" can have this part and "See traces" should have collector part.
OpenTelemetry is configured on both the client and the server, and exports to | ||
the Prometheus exporter. The exporter exposes metrics on the Prometheus ports | ||
described above. | ||
4. **View Traces in the Collector Output:** |
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.
Can't we do curl
similar to metrics?
The client continuously makes RPC's to a server. The client and server both | ||
expose a prometheus exporter to listen and provide metrics. This defaults to | ||
:9464 for the server and :9465 for the client. | ||
* **Local Collector:** |
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.
Does this need to be running before client and server? If yes, we need to move this up and and mention in brackets (Only for traces)
|
||
Curling to the exposed Prometheus ports outputs the metrics recorded on the |
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.
Same. We can't remove this. As this is for metrics.
@@ -0,0 +1,19 @@ | |||
receivers: |
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.
Please link a URL at the top for collector config format
} | ||
spanProcessor := trace.NewSimpleSpanProcessor(traceExporter) | ||
tp := trace.NewTracerProvider(trace.WithSpanProcessor(spanProcessor), trace.WithResource(res)) | ||
otel.SetTracerProvider(tp) |
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.
@dfawley was there any reason we decided to set these outside of gRPC? If we are asking user to set these, do we need it to be in TraceOptions?
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.
We don't really want to be using this global in our implementation. We should be able to delete this line since we should be using the TracerProvider
in TraceOptions
. Global state should generally be avoided.
if err != nil { | ||
log.Fatalf("Failed to create otlp trace exporter: %v", err) | ||
} | ||
// resource.New adds service metadata to telemetry, enabling context and |
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.
is this mandatory for the tracing to work? If not, we don't need to add here
"go.opentelemetry.io/otel/sdk/metric" | ||
"go.opentelemetry.io/otel/sdk/resource" | ||
"go.opentelemetry.io/otel/sdk/trace" |
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.
I would prefer to rename these imports as otelmetric
/etc so there is no confusion. "trace" and "metric" are such generic names that it could be confusing.
} | ||
spanProcessor := trace.NewSimpleSpanProcessor(traceExporter) | ||
tp := trace.NewTracerProvider(trace.WithSpanProcessor(spanProcessor), trace.WithResource(res)) | ||
otel.SetTracerProvider(tp) |
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.
We don't really want to be using this global in our implementation. We should be able to delete this line since we should be using the TracerProvider
in TraceOptions
. Global state should generally be avoided.
RELEASE NOTE: