-
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 4 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,7 +4,10 @@ | |||||
| package main | ||||||
|
|
||||||
| import ( | ||||||
| "io" | ||||||
| "context" | ||||||
| "encoding/json" | ||||||
| "errors" | ||||||
| "log" | ||||||
lightsta1ker marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| "math/rand" | ||||||
| "net/http" | ||||||
| "strconv" | ||||||
|
|
@@ -19,42 +22,141 @@ 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 int64 | ||||||
flc1125 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| ) | ||||||
|
|
||||||
| 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"), |
lightsta1ker marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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
}
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.
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.