-
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 13 commits
f89e2b5
a337ba6
da2e8cd
08b7f61
6697966
ddf3c68
ccea55d
fbacc37
a474139
6d00f40
c066c03
ae85955
96f2473
05421c3
2edd445
06b7cc6
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,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"), | ||||||||||||||||||||
|
||||||||||||||||||||
| metric.WithDescription("The last rolls value observed"), | |
| metric.WithDescription("The number of dice rolled in the most recent request"), |
pellared marked this conversation as resolved.
Show resolved
Hide resolved
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.
Proper error handling is missing here (500 Internal Server Error).
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.
@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"} ?
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 mean if the json encoding fails then it is an internal server error.
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 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
}
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.
Hi @pellared ,
Please help understand how to properly handle the encoding failure.
Thank you.
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 am very sorry but I have no time for this. Note that Copilot also have found some bugs in this PR.
pellared marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
pellared marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
pellared marked this conversation as resolved.
Show resolved
Hide resolved
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] 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.
| 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.") |
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.
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.
pellared marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Fixed
Show fixed
Hide fixed
flc1125 marked this conversation as resolved.
Show resolved
Hide resolved
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.
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))
| rollCnt.Add(ctx, 1, metric.WithAttributes(rollsAttr)) | |
| rollCnt.Add(ctx, int64(rolls), metric.WithAttributes(rollsAttr)) |
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.
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.