Updating dice example based on Updated Specification for the reference application
#8099
Conversation
lightsta1ker
changed the title
dice exampledice example based on Updated Specification for the reference application
lightsta1ker
force-pushed
the
dice/issue-7937
branch
from
ddae343 to
a337ba6
Compare
lightsta1ker
changed the title
dice example based on Updated Specification for the reference applicationdice example based on Updated Specification for the reference application
|
@lightsta1ker, I won't have time to review it further until I am back from KubeCon NA. Hopefully other @open-telemetry/go-approvers are going to help with reviews. |
|
Sure, @pellared . Thank you for the review & suggestions! |
| "encoding/json" | ||
| "errors" | ||
| "log" | ||
| "math/rand" |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@lightsta1ker We still have some issues to resolve, including CI checks. |
|
@flc1125 I thought those checks were not relevant to this PR as it's not customer facing. |
They are relevant. |
|
Updated with fixes for failing lint checks and error handling. Also rebased. |
|
@pellared Is changelog needed? |
No |
pellared
added
the
Skip Changelog
There was a problem hiding this comment.
Pull Request Overview
This PR updates the dice example application to align with the updated specification for the OpenTelemetry reference application. The changes modernize the API design, improve observability instrumentation, and add configurable deployment options.
Key Changes
- Refactored HTTP handler to use query parameters (
rollsandplayer) instead of path parameters, with JSON responses - Enhanced OpenTelemetry instrumentation with additional metrics (histogram for dice outcomes, gauge for last rolls value) and proper resource configuration
- Added configurable port via
APPLICATION_PORTenvironment variable for flexible deployment
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/dice/uninstrumented/rolldice.go | Refactored handler to parse query params, return JSON responses, added input validation with max rolls limit, and separated concerns with helper functions |
| examples/dice/uninstrumented/main.go | Added configurable port support and updated route registration to single /rolldice endpoint |
| examples/dice/instrumented/rolldice.go | Applied same refactoring as uninstrumented version, upgraded to math/rand/v2, added histogram and gauge metrics, improved telemetry context propagation |
| examples/dice/instrumented/otel.go | Added resource configuration with environment, process, host, and SDK information; updated all provider constructors to accept resource parameter |
| examples/dice/instrumented/main.go | Added configurable port support and simplified route registration to single endpoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@open-telemetry/go-approvers, anyone has some time to review this PR? |
Co-authored-by: Damien Mathieu <42@dmathieu.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func writeJSON(w http.ResponseWriter, v any) { | ||
| data, err := json.Marshal(v) | ||
| if err != nil { | ||
| w.Header().Set("Content-Type", "application/json") |
There was a problem hiding this comment.
The Content-Type header is set after WriteHeader is called. Headers must be set before calling WriteHeader to ensure they are properly sent in the response.
| results := make([]int, rolls) | ||
| for i := range rolls { |
There was a problem hiding this comment.
The uninstrumented version has special handling when rolls==1 (lines 88-89) that returns early without allocating a slice and using a loop. However, the instrumented version doesn't have this optimization and always allocates a slice and uses a loop even for a single roll. This creates an inconsistency in behavior between the two versions.
| results := make([]int, rolls) | |
| for i := range rolls { | |
| if rolls == 1 { | |
| // Fast path for a single roll to mirror the uninstrumented implementation. | |
| result := rollOnce(ctx) | |
| outcomeHist.Record(ctx, int64(result)) | |
| rollsAttr := attribute.Int("rolls", rolls) | |
| span.SetAttributes(rollsAttr) | |
| rollCnt.Add(ctx, int64(rolls), metric.WithAttributes(rollsAttr)) | |
| lastRolls.Store(int64(rolls)) | |
| return []int{result}, nil | |
| } | |
| results := make([]int, rolls) | |
| for i := range results { |
| 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.") |
There was a problem hiding this comment.
The log message "Some player rolled a dice." is vague and doesn't provide useful information. The DEBUG logs above (lines 111-113) already capture the actual roll information with player details and results. This INFO log should either be removed or made more informative, such as including the HTTP method and URL like the uninstrumented version does at line 51.
| logger.InfoContext(r.Context(), "Some player rolled a dice.") | |
| logger.InfoContext(r.Context(), "player rolled dice request", "method", r.Method, "url", r.URL.String()) |
| if err != nil { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| msg := "Internal server error" |
There was a problem hiding this comment.
The error message is inconsistent between instrumented and uninstrumented versions. The uninstrumented version returns a detailed error message from the rollDice function (e.g., "rolls parameter exceeds maximum allowed value"), while the instrumented version returns a generic "Internal server error" message. This reduces the usefulness of error responses for API consumers.
| msg := "Internal server error" | |
| msg := err.Error() |
| logger.InfoContext(ctx, msg, "result", roll) | ||
| logger.InfoContext(r.Context(), "Some player rolled a dice.") | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") |
There was a problem hiding this comment.
The Content-Type header is set twice for successful responses - once at line 117 before calling writeJSON, and again inside writeJSON at line 72. The duplicate header setting at line 117 should be removed since writeJSON already sets it.
| w.Header().Set("Content-Type", "application/json") |
| _, 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. |
There was a problem hiding this comment.
The instrumented version uses math/rand/v2 and rand.IntN, while the uninstrumented version uses math/rand and rand.Intn. For consistency and to serve as a proper reference example, both versions should use the same random number generation approach.
|
|
||
| results, err := rolldice(rolls) | ||
| if err != nil { | ||
| w.Header().Set("Content-Type", "application/json") |
There was a problem hiding this comment.
The Content-Type header is set after WriteHeader is called. Headers should be set before calling WriteHeader, otherwise they may not be properly sent in the response.
|
|
||
| // Check if rolls is a number. | ||
| rolls, err := strconv.Atoi(rollsParam) | ||
| if err != nil { |
There was a problem hiding this comment.
Missing Content-Type header before WriteHeader. The header should be set before calling WriteHeader to ensure it's properly included in the response. This is inconsistent with how the instrumented version handles this at line 86-87, and with how other error paths in this file handle it.
| if err != nil { | |
| if err != nil { | |
| w.Header().Set("Content-Type", "application/json") |
| results, err := rolldice(rolls) | ||
| if err != nil { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusInternalServerError) |
There was a problem hiding this comment.
Missing JSON response body for the error. When rolldice returns an error, the response should include a JSON error message body like other error paths in this handler do (e.g., lines 30-32). Currently, it only sets headers and returns, resulting in an empty response body.
| w.WriteHeader(http.StatusInternalServerError) | |
| w.WriteHeader(http.StatusInternalServerError) | |
| _ = json.NewEncoder(w).Encode(map[string]string{ | |
| "status": "error", | |
| "message": "Internal Server Error", | |
| }) |
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusInternalServerError) |
There was a problem hiding this comment.
The Content-Type header is set after WriteHeader is called. Headers should be set before calling WriteHeader to ensure they are properly sent in the response.
5 participants
Addresses #7937