-
Notifications
You must be signed in to change notification settings - Fork 707
Updating dice example based on Updated Specification for the reference application
#8099
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
base: main
Are you sure you want to change the base?
Changes from all commits
f89e2b5
a337ba6
da2e8cd
08b7f61
6697966
ddf3c68
ccea55d
fbacc37
a474139
6d00f40
c066c03
ae85955
96f2473
05421c3
2edd445
06b7cc6
4440f23
837eca3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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" | ||||||||||||||||||||
|
|
@@ -19,42 +22,163 @@ 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"), | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| 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) { | ||||||||||||||||||||
| // 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" | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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. | ||||||||||||||||||||
| // 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) | ||||||||||||||||||||
pellared marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
| msg := "Parameter rolls must be a positive integer" | ||||||||||||||||||||
| _ = json.NewEncoder(w).Encode(map[string]string{ | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Proper error handling is missing here (500 Internal Server Error).
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pellared Isn't it refering to this line -
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @pellared ,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| var msg string | ||||||||||||||||||||
| if player := r.PathValue("player"); player != "" { | ||||||||||||||||||||
| msg = player + " is rolling the dice" | ||||||||||||||||||||
| results, err := rollDice(r.Context(), rolls) | ||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||
| w.Header().Set("Content-Type", "application/json") | ||||||||||||||||||||
| w.WriteHeader(http.StatusInternalServerError) | ||||||||||||||||||||
| msg := "Internal server error" | ||||||||||||||||||||
| _ = json.NewEncoder(w).Encode(map[string]string{ | ||||||||||||||||||||
| "status": "error", | ||||||||||||||||||||
| "message": msg, | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| logger.ErrorContext(r.Context(), err.Error()) | ||||||||||||||||||||
pellared marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
| return | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if player == "" { | ||||||||||||||||||||
| logger.DebugContext(r.Context(), "anonymous player rolled", "results", results) | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| msg = "Anonymous player is rolling the dice" | ||||||||||||||||||||
| logger.DebugContext(r.Context(), "player rolled dice", "player", player, "results", results) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| logger.InfoContext(ctx, msg, "result", roll) | ||||||||||||||||||||
| logger.InfoContext(r.Context(), "Some player rolled a dice.") | ||||||||||||||||||||
|
||||||||||||||||||||
| 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
AI
Nov 19, 2025
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.
[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.
| logger.InfoContext(r.Context(), "Some player rolled a dice.") |
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.
[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.