Skip to content
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
9 changes: 6 additions & 3 deletions examples/dice/instrumented/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,12 @@ func run() error {
}()

// Start HTTP server.
port := os.Getenv("APPLICATION_PORT")
if port == "" {
port = "8080"
}
srv := &http.Server{
Addr: ":8080",
Addr: ":" + port,
BaseContext: func(net.Listener) context.Context { return ctx },
ReadTimeout: time.Second,
WriteTimeout: 10 * time.Second,
Expand Down Expand Up @@ -72,8 +76,7 @@ func newHTTPHandler() http.Handler {
mux := http.NewServeMux()

// Register handlers.
mux.Handle("/rolldice", http.HandlerFunc(rolldice))
mux.Handle("/rolldice/{player}", http.HandlerFunc(rolldice))
mux.Handle("/rolldice", http.HandlerFunc(handleRolldice))

// Add HTTP instrumentation for the whole server.
handler := otelhttp.NewHandler(mux, "/")
Expand Down
34 changes: 20 additions & 14 deletions examples/dice/instrumented/otel.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/sdk/log"
"go.opentelemetry.io/otel/sdk/metric"
"go.opentelemetry.io/otel/sdk/resource"
"go.opentelemetry.io/otel/sdk/trace"
)

Expand All @@ -42,12 +43,19 @@ func setupOTelSDK(ctx context.Context) (func(context.Context) error, error) {
err = errors.Join(inErr, shutdown(ctx))
}

res, _ := resource.New(ctx,
resource.WithFromEnv(), // reads OTEL_SERVICE_NAME & OTEL_RESOURCE_ATTRIBUTES.
resource.WithProcess(),
resource.WithHost(),
resource.WithTelemetrySDK(),
)
Comment on lines +46 to +51
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The error returned by resource.New() is being silently ignored. This could lead to using a nil or incomplete resource, potentially causing issues with telemetry data. Either handle the error properly or at minimum log it if you intend to continue with a partial resource.

Suggested change
res, _ := resource.New(ctx,
resource.WithFromEnv(), // reads OTEL_SERVICE_NAME & OTEL_RESOURCE_ATTRIBUTES.
resource.WithProcess(),
resource.WithHost(),
resource.WithTelemetrySDK(),
)
res, resErr := resource.New(ctx,
resource.WithFromEnv(), // reads OTEL_SERVICE_NAME & OTEL_RESOURCE_ATTRIBUTES.
resource.WithProcess(),
resource.WithHost(),
resource.WithTelemetrySDK(),
)
if resErr != nil {
handleErr(resErr)
return shutdown, resErr
}

Copilot uses AI. Check for mistakes.

// Set up propagator.
prop := newPropagator()
otel.SetTextMapPropagator(prop)

// Set up trace provider.
tracerProvider, err := newtracerProvider()
tracerProvider, err := newTracerProvider(res)
if err != nil {
handleErr(err)
return shutdown, err
Expand All @@ -56,7 +64,7 @@ func setupOTelSDK(ctx context.Context) (func(context.Context) error, error) {
otel.SetTracerProvider(tracerProvider)

// Set up meter provider.
meterProvider, err := newMeterProvider()
meterProvider, err := newMeterProvider(res)
if err != nil {
handleErr(err)
return shutdown, err
Expand All @@ -65,7 +73,7 @@ func setupOTelSDK(ctx context.Context) (func(context.Context) error, error) {
otel.SetMeterProvider(meterProvider)

// Set up logger provider.
loggerProvider, err := newLoggerProvider()
loggerProvider, err := newLoggerProvider(res)
if err != nil {
handleErr(err)
return shutdown, err
Expand All @@ -83,43 +91,41 @@ func newPropagator() propagation.TextMapPropagator {
)
}

func newtracerProvider() (*trace.TracerProvider, error) {
traceExporter, err := stdouttrace.New(
stdouttrace.WithPrettyPrint())
func newTracerProvider(res *resource.Resource) (*trace.TracerProvider, error) {
traceExporter, err := stdouttrace.New(stdouttrace.WithPrettyPrint())
if err != nil {
return nil, err
}

tracerProvider := trace.NewTracerProvider(
trace.WithBatcher(traceExporter,
// Default is 5s. Set to 1s for demonstrative purposes.
trace.WithBatchTimeout(time.Second)),
trace.WithBatcher(traceExporter, trace.WithBatchTimeout(time.Second)), // Default is 5s. Set to 1s for demonstrative purposes.
trace.WithResource(res),
)
return tracerProvider, nil
}

func newMeterProvider() (*metric.MeterProvider, error) {
func newMeterProvider(res *resource.Resource) (*metric.MeterProvider, error) {
metricExporter, err := stdoutmetric.New()
if err != nil {
return nil, err
}

meterProvider := metric.NewMeterProvider(
metric.WithReader(metric.NewPeriodicReader(metricExporter,
// Default is 1m. Set to 3s for demonstrative purposes.
metric.WithInterval(3*time.Second))),
metric.WithReader(metric.NewPeriodicReader(metricExporter, metric.WithInterval(3*time.Second))), // Default is 1m. Set to 3s for demonstrative purposes.
metric.WithResource(res),
)
return meterProvider, nil
}

func newLoggerProvider() (*log.LoggerProvider, error) {
func newLoggerProvider(res *resource.Resource) (*log.LoggerProvider, error) {
logExporter, err := stdoutlog.New()
if err != nil {
return nil, err
}

loggerProvider := log.NewLoggerProvider(
log.WithProcessor(log.NewBatchProcessor(logExporter)),
log.WithResource(res),
)
return loggerProvider, nil
}
152 changes: 131 additions & 21 deletions examples/dice/instrumented/rolldice.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@
package main

import (
"io"
"math/rand"
"context"
"encoding/json"
"errors"
"math/rand/v2"
"net/http"
"strconv"
"sync/atomic"

"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
Expand All @@ -19,42 +22,149 @@ import (
const name = "go.opentelemetry.io/contrib/examples/dice"

var (
tracer = otel.Tracer(name)
meter = otel.Meter(name)
logger = otelslog.NewLogger(name)
rollCnt metric.Int64Counter
tracer = otel.Tracer(name)
meter = otel.Meter(name)
logger = otelslog.NewLogger(name)
rollCnt metric.Int64Counter
outcomeHist metric.Int64Histogram
lastRollsGauge metric.Int64ObservableGauge
lastRolls atomic.Int64
)

func init() {
var err error
rollCnt, err = meter.Int64Counter("dice.rolls",
metric.WithDescription("The number of rolls by roll value"),
metric.WithDescription("The number of rolls"),
metric.WithUnit("{roll}"))
if err != nil {
panic(err)
}

outcomeHist, err = meter.Int64Histogram(
"dice.outcome",
metric.WithDescription("Distribution of dice outcomes (1-6)"),
metric.WithUnit("{count}"),
)
if err != nil {
panic(err)
}

lastRollsGauge, err = meter.Int64ObservableGauge(
"dice.last.rolls",
metric.WithDescription("The last rolls value observed"),
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The description "The last rolls value observed" is unclear. Consider clarifying what "rolls value" means, e.g., "The number of dice rolled in the most recent request" to better describe what this gauge represents.

Suggested change
metric.WithDescription("The last rolls value observed"),
metric.WithDescription("The number of dice rolled in the most recent request"),

Copilot uses AI. Check for mistakes.
)
if err != nil {
panic(err)
}

// Register the gauge callback.
_, err = meter.RegisterCallback(
func(_ context.Context, o metric.Observer) error {
o.ObserveInt64(lastRollsGauge, lastRolls.Load())
return nil
},
lastRollsGauge,
)
if err != nil {
panic(err)
}
}

func rolldice(w http.ResponseWriter, r *http.Request) {
ctx, span := tracer.Start(r.Context(), "roll")
defer span.End()
func handleRolldice(w http.ResponseWriter, r *http.Request) {
const maxRolls = 1000 // Arbitrary limit to prevent Slice memory allocation with excessive size value.

// Parse query parameters.
rollsParam := r.URL.Query().Get("rolls")
player := r.URL.Query().Get("player")

// Default rolls = 1 if not defined.
if rollsParam == "" {
rollsParam = "1"
}

// Check if rolls is a number.
rolls, err := strconv.Atoi(rollsParam)
if err != nil {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusBadRequest)
msg := "Parameter rolls must be a positive integer"
_ = json.NewEncoder(w).Encode(map[string]string{
Copy link
Member

@pellared pellared Nov 19, 2025

Choose a reason for hiding this comment

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

Proper error handling is missing here (500 Internal Server Error).

Copy link
Author

Choose a reason for hiding this comment

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

@pellared Isn't it refering to this line -
if rolls is set to an invalid input (not a number): status code 400 and {"status": "error", "message": "Parameter rolls must be a positive integer"} ?

Copy link
Member

Choose a reason for hiding this comment

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

I mean if the json encoding fails then it is an internal server error.

Copy link
Author

Choose a reason for hiding this comment

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

I thought that the chances of static map encoding failure is very less. Can I handle it like this?

if err != nil {
    w.Header().Set("Content-Type", "application/json")
    w.WriteHeader(http.StatusBadRequest)

    msg := "Parameter rolls must be a positive integer"
    resp := map[string]string{
        "status":  "error",
        "message": msg,
    }

    if encErr := json.NewEncoder(w).Encode(resp); encErr != nil {
        logger.ErrorContext(r.Context(), "failed to write error JSON", "error", encErr)
        http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
        return
    }

    logger.WarnContext(r.Context(), msg)
    return
}

Copy link
Author

Choose a reason for hiding this comment

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

Hi @pellared ,
Please help understand how to properly handle the encoding failure.
Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

I am very sorry but I have no time for this. Note that Copilot also have found some bugs in this PR.

"status": "error",
"message": msg,
})
logger.WarnContext(r.Context(), msg)
return
}

if rolls > maxRolls {
w.WriteHeader(http.StatusInternalServerError)
logger.ErrorContext(r.Context(), "rolls parameter exceeds maximum allowed value")
return
}

roll := 1 + rand.Intn(6) //nolint:gosec // G404: Use of weak random number generator (math/rand instead of crypto/rand) is ignored as this is not security-sensitive.
results, err := rollDice(r.Context(), rolls)
if err != nil {
// Signals invalid input (<=0).
w.WriteHeader(http.StatusInternalServerError)
logger.ErrorContext(r.Context(), err.Error())
return
}

if player == "" {
logger.DebugContext(r.Context(), "anonymous player rolled", "results", results)
} else {
logger.DebugContext(r.Context(), "player rolled dice", "player", player, "results", results)
}
logger.InfoContext(r.Context(), "Some player rolled a dice.")
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

[nitpick] Inconsistent logging format compared to the uninstrumented version. The uninstrumented version logs request details like "INFO: GET /rolldice?rolls=5 -> 200 OK" (line 50), while the instrumented version logs a generic message. Consider maintaining similar informative logging for consistency across both examples.

Suggested change
logger.InfoContext(r.Context(), "Some player rolled a dice.")
logger.InfoContext(
r.Context(),
"Request handled",
"method", r.Method,
"path", r.URL.Path+"?"+r.URL.RawQuery,
"status", "200 OK",
)

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The log message "Some player rolled a dice" is generic and doesn't add useful information beyond what's already logged in lines 105-108. Consider either making this message more specific (e.g., include the request status or outcome summary) or removing it to avoid redundant logging.

Suggested change
logger.InfoContext(r.Context(), "Some player rolled a dice.")

Copilot uses AI. Check for mistakes.

var msg string
if player := r.PathValue("player"); player != "" {
msg = player + " is rolling the dice"
w.Header().Set("Content-Type", "application/json")
if len(results) == 1 {
writeJSON(r.Context(), w, results[0])
} else {
msg = "Anonymous player is rolling the dice"
writeJSON(r.Context(), w, results)
}
logger.InfoContext(ctx, msg, "result", roll)
}

func writeJSON(ctx context.Context, w http.ResponseWriter, v any) {
if err := json.NewEncoder(w).Encode(v); err != nil {
w.WriteHeader(http.StatusInternalServerError)
logger.ErrorContext(ctx, "json encode failed", "error", err)
}
Comment on lines +119 to +123
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The HTTP status code should be set before writing the response body. Setting WriteHeader after json.NewEncoder(w).Encode() has already written data will have no effect, as headers are sent with the first write. Move the WriteHeader call before the Encode call.

Copilot uses AI. Check for mistakes.
}

// rollDice is the outer function which Does the error handling.
func rollDice(ctx context.Context, rolls int) ([]int, error) {
ctx, span := tracer.Start(ctx, "rollDice")
defer span.End()

if rolls <= 0 {
err := errors.New("rolls must be positive")
span.RecordError(err)
return nil, err
}

results := make([]int, rolls)
for i := range rolls {
results[i] = rollOnce(ctx)
outcomeHist.Record(ctx, int64(results[i]))
}

rollsAttr := attribute.Int("rolls", rolls)
span.SetAttributes(rollsAttr)
rollCnt.Add(ctx, 1, metric.WithAttributes(rollsAttr))
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The counter is being incremented by 1 regardless of the number of rolls performed. This doesn't accurately represent the total number of dice rolls. Consider incrementing by the number of actual rolls: rollCnt.Add(ctx, int64(rolls), metric.WithAttributes(rollsAttr))

Suggested change
rollCnt.Add(ctx, 1, metric.WithAttributes(rollsAttr))
rollCnt.Add(ctx, int64(rolls), metric.WithAttributes(rollsAttr))

Copilot uses AI. Check for mistakes.
lastRolls.Store(int64(rolls))
return results, nil
}

// rollOnce is the inner function — returns a random number 1–6.
func rollOnce(ctx context.Context) int {
_, span := tracer.Start(ctx, "rollOnce")
defer span.End()

roll := 1 + rand.IntN(6) //nolint:gosec // G404: Use of weak random number generator (math/rand instead of crypto/rand) is ignored as this is not security-sensitive.

rollValueAttr := attribute.Int("roll.value", roll)
span.SetAttributes(rollValueAttr)
rollCnt.Add(ctx, 1, metric.WithAttributes(rollValueAttr))

resp := strconv.Itoa(roll) + "\n"
if _, err := io.WriteString(w, resp); err != nil {
logger.ErrorContext(ctx, "Write failed", "error", err)
}
return roll
}
11 changes: 7 additions & 4 deletions examples/dice/uninstrumented/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,13 @@ func run() (err error) {
defer stop()

// Start HTTP server.
port := os.Getenv("APPLICATION_PORT")
if port == "" {
port = "8080"
}

srv := &http.Server{
Addr: ":8080",
Addr: ":" + port,
BaseContext: func(net.Listener) context.Context { return ctx },
ReadTimeout: time.Second,
WriteTimeout: 10 * time.Second,
Expand Down Expand Up @@ -60,8 +65,6 @@ func newHTTPHandler() http.Handler {
mux := http.NewServeMux()

// Register handlers.
mux.HandleFunc("/rolldice/", rolldice)
mux.HandleFunc("/rolldice/{player}", rolldice)

mux.HandleFunc("/rolldice", handleRolldice)
return mux
}
Loading
Loading