-
Notifications
You must be signed in to change notification settings - Fork 706
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 5 commits
f89e2b5
a337ba6
da2e8cd
08b7f61
6697966
ddf3c68
ccea55d
fbacc37
a474139
6d00f40
c066c03
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,9 @@ | |||||
| package main | ||||||
|
|
||||||
| import ( | ||||||
| "io" | ||||||
| "context" | ||||||
| "encoding/json" | ||||||
| "errors" | ||||||
| "math/rand" | ||||||
| "net/http" | ||||||
| "strconv" | ||||||
|
|
@@ -19,42 +21,141 @@ | |||||
| 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"), | ||||||
| ) | ||||||
| if err != nil { | ||||||
| panic(err) | ||||||
| } | ||||||
|
|
||||||
| // Register the gauge callback. | ||||||
| _, err = meter.RegisterCallback( | ||||||
| func(ctx context.Context, o metric.Observer) error { | ||||||
| o.ObserveInt64(lastRollsGauge, lastRolls) | ||||||
| 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") | ||||||
|
|
||||||
| 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. | ||||||
| // 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.WriteHeader(http.StatusBadRequest) | ||||||
|
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. what about Content-Type?
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. Thought of leaving it to the defaults as it's a simple app and not explicitly specified in the reference. But I will set |
||||||
| msg := "Parameter rolls must be a positive integer" | ||||||
| _ = json.NewEncoder(w).Encode(map[string]string{ | ||||||
| "status": "error", | ||||||
| "message": msg, | ||||||
| }) | ||||||
| logger.WarnContext(r.Context(), msg) | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| results, err := rollDice(r.Context(), rolls) | ||||||
| if err != nil { | ||||||
| // Signals invalid input (<=0). | ||||||
| w.WriteHeader(http.StatusInternalServerError) | ||||||
| logger.ErrorContext(r.Context(), err.Error()) | ||||||
|
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. Since the condition
|
||||||
| return | ||||||
| } | ||||||
|
|
||||||
| var msg string | ||||||
| if player := r.PathValue("player"); player != "" { | ||||||
| msg = player + " is rolling the dice" | ||||||
| if player == "" { | ||||||
| logger.DebugContext(r.Context(), "anonymous player rolled", results) | ||||||
| } else { | ||||||
| msg = "Anonymous player is rolling the dice" | ||||||
| logger.DebugContext(r.Context(), "player=%s rolled %v", player, results) | ||||||
| } | ||||||
|
Comment on lines
107
to
111
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. Can we use log attributes? |
||||||
| logger.InfoContext(r.Context(), " %s %s -> 200 OK", r.Method, r.URL.String()) | ||||||
|
||||||
| logger.InfoContext(r.Context(), " %s %s -> 200 OK", r.Method, r.URL.String()) | |
| 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.
@svrnm, maybe you have some recommendation.
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've just kept the "200 OK" to fullfill the "an INFO-level message for each HTTP request with a status code <400". . But it could simply be the above suggestion. Will update.
Outdated
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.
Isn't the caller already logging the error ?
| logger.ErrorContext(ctx, "error", "error", err) |
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.
Ah, Yes. The caller does log the same error at line 100
logger.ErrorContext(r.Context(), err.Error())
I think, this line was from earlier that escaped refactor. Will update.
Check failure
Code scanning / CodeQL
Slice memory allocation with excessive size value High
Uh oh!
There was an error while loading. Please reload this page.