Updating `dice` example based on Updated Specification for the reference application #8099

lightsta1ker · 2025-10-30T12:59:37Z

Addresses #7937

examples/dice/instrumented/main.go

examples/dice/uninstrumented/main.go

examples/dice/instrumented/main.go

examples/dice/instrumented/rolldice.go

examples/dice/uninstrumented/rolldice.go

examples/dice/instrumented/rolldice.go

pellared · 2025-11-04T11:03:28Z

examples/dice/instrumented/rolldice.go

+	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)
+	}


Can we use log attributes?

pellared · 2025-11-04T11:05:12Z

examples/dice/instrumented/rolldice.go

-		msg = "Anonymous player is rolling the dice"
+		logger.DebugContext(r.Context(), "player=%s rolled %v", player, results)
+	}
+	logger.InfoContext(r.Context(), " %s %s -> 200 OK", r.Method, r.URL.String())


It is it what other languages are doing? I do not see such requirements in https://opentelemetry.io/docs/getting-started/reference-application-specification/ apart from "an INFO-level message for each HTTP request with a status code <400".

This could simply be

Suggested change

logger.InfoContext(r.Context(), " %s %s -> 200 OK", r.Method, r.URL.String())

logger.InfoContext(r.Context(), "Some player rolled a dice.")

@svrnm, maybe you have some recommendation.

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.

pellared · 2025-11-04T11:05:53Z

examples/dice/instrumented/rolldice.go

+	// Check if rolls is a number.
+	rolls, err := strconv.Atoi(rollsParam)
+	if err != nil {
+		w.WriteHeader(http.StatusBadRequest)


what about Content-Type?

Thought of leaving it to the defaults as it's a simple app and not explicitly specified in the reference. But I will set application/json as that's how the app responds.

pellared · 2025-11-04T11:09:58Z

examples/dice/instrumented/rolldice.go

+	if rolls <= 0 {
+		err := errors.New("rolls must be positive")
+		span.RecordError(err)
+		logger.ErrorContext(ctx, "error", "error", err)


Isn't the caller already logging the error ?

Suggested change

logger.ErrorContext(ctx, "error", "error", err)

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.

pellared · 2025-11-04T11:11:37Z

@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.

lightsta1ker · 2025-11-04T11:14:09Z

Sure, @pellared . Thank you for the review & suggestions!

examples/dice/instrumented/otel.go

examples/dice/uninstrumented/rolldice.go

examples/dice/instrumented/rolldice.go

+		return []int{val}, nil
+	}
+
+	results := make([]int, rolls)


examples/dice/uninstrumented/rolldice.go

+		return []int{rollOnce()}, nil
+	}
+
+	results := make([]int, rolls)


flc1125 · 2025-11-05T01:07:35Z

examples/dice/instrumented/rolldice.go

+		rollsAttr := attribute.Int("rolls", rolls)
+		span.SetAttributes(rollsAttr)
+		rollCnt.Add(ctx, 1, metric.WithAttributes(rollsAttr))
+		outcomeHist.Record(ctx, int64(val))


Can we consolidate the logic of this part? Currently, it contains the same logic twice.

Yes, the loop handles rolls == 1 just fine. Thank you, will update.

flc1125

The normative review is completed, with additional notes:

According to the specification requirements, support for the OTLP exporter is needed. Currently, this part is not yet supported, but I believe it can be fully implemented through a separate PR.

flc1125 · 2025-11-05T04:56:51Z

examples/dice/instrumented/rolldice.go

+	// Parse query parameters.
+	rollsParam := r.URL.Query().Get("rolls")
+	player := r.URL.Query().Get("player")
+	maxRolls := 1000 // Arbitrary limit to prevent Slice memory allocation with excessive size value.


From a performance perspective, this is a good configuration.

However, I am considering that since the specification does not mention this situation and it is just an example.

Therefore, extreme cases can be temporarily ignored. I recommend removing it to comply with the specification.

I kindly disagree. We should not have vulnerabilities in examples. People are using them for learning purposes.

The codeQL CI check picked up the vulnerability. I would like to keep the example vulnerability free as well.

I think it can be kept, but defining it as a constant is a better choice.

flc1125 · 2025-11-05T04:58:30Z

examples/dice/instrumented/rolldice.go

+	if err != nil || rolls > maxRolls {
+		// Signals invalid input (<=0).
+		w.WriteHeader(http.StatusInternalServerError)
+		logger.ErrorContext(r.Context(), err.Error())


Since the condition if err != nil || rolls > maxRolls { implies that err might be nil.

~~Of course, once maxRolls is removed, this issue is naturally resolved.~~

flc1125 · 2025-11-11T13:02:03Z

@lightsta1ker We still have some issues to resolve, including CI checks.

lightsta1ker · 2025-11-11T13:13:29Z

@flc1125 I thought those checks were not relevant to this PR as it's not customer facing.

lightsta1ker changed the title ~~[WIP] Updating uninstrumented dice example~~ [WIP] Updating dice example based on Updated Specification for the reference application Oct 30, 2025

lightsta1ker added 2 commits October 30, 2025 23:12

Updating uninstrumented example

f89e2b5

Updating instrumented example

a337ba6

lightsta1ker force-pushed the dice/issue-7937 branch from ddae343 to a337ba6 Compare October 30, 2025 17:43

lightsta1ker marked this pull request as ready for review October 30, 2025 17:43

lightsta1ker requested a review from a team as a code owner October 30, 2025 17:43

lightsta1ker changed the title ~~[WIP] Updating dice example based on Updated Specification for the reference application~~ Updating dice example based on Updated Specification for the reference application Oct 31, 2025

pellared reviewed Nov 4, 2025

View reviewed changes

examples/dice/instrumented/main.go Outdated Show resolved Hide resolved

examples/dice/instrumented/main.go Outdated Show resolved Hide resolved

examples/dice/uninstrumented/main.go Outdated Show resolved Hide resolved

examples/dice/instrumented/main.go Outdated Show resolved Hide resolved

lightsta1ker added 2 commits November 4, 2025 15:27

chore: lint and suggestions

da2e8cd

Merge branch 'main' into dice/issue-7937

08b7f61

pellared reviewed Nov 4, 2025

View reviewed changes

examples/dice/instrumented/rolldice.go Outdated Show resolved Hide resolved

pellared reviewed Nov 4, 2025

View reviewed changes

examples/dice/uninstrumented/rolldice.go Outdated Show resolved Hide resolved

examples/dice/instrumented/rolldice.go Outdated Show resolved Hide resolved

chore: rebase & refactor suggestions

6697966

pellared reviewed Nov 4, 2025

View reviewed changes

flc1125 reviewed Nov 4, 2025

View reviewed changes

examples/dice/instrumented/otel.go Outdated Show resolved Hide resolved

examples/dice/instrumented/otel.go Outdated Show resolved Hide resolved

flc1125 reviewed Nov 4, 2025

View reviewed changes

examples/dice/uninstrumented/rolldice.go Show resolved Hide resolved

flc1125 reviewed Nov 4, 2025

View reviewed changes

examples/dice/instrumented/rolldice.go Outdated Show resolved Hide resolved

flc1125 reviewed Nov 4, 2025

View reviewed changes

examples/dice/instrumented/rolldice.go Show resolved Hide resolved

github-advanced-security bot found potential problems Nov 4, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

chore: refactor suggestions

ddf3c68

This comment was marked as resolved.

Sign in to view

lightsta1ker added 2 commits November 4, 2025 22:29

Merge branch 'main' into dice/issue-7937

ccea55d

fix: Limiting memory allocation for rolls

fbacc37

flc1125 reviewed Nov 5, 2025

View reviewed changes

lightsta1ker added 2 commits November 5, 2025 15:39

Merge branch 'main' into dice/issue-7937

a474139

chore: consolidating rollDice observability logic

6d00f40

chore: refactoring suggestion

c066c03

lightsta1ker requested review from flc1125 and pellared November 11, 2025 09:20

	logger.InfoContext(r.Context(), " %s %s -> 200 OK", r.Method, r.URL.String())
	logger.InfoContext(r.Context(), "Some player rolled a dice.")

Updating dice example based on Updated Specification for the reference application #8099

Are you sure you want to change the base?

Updating dice example based on Updated Specification for the reference application #8099

Conversation

lightsta1ker commented Oct 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pellared commented Nov 4, 2025

Uh oh!

lightsta1ker commented Nov 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check failure

Uh oh!

Check failure

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flc1125 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pellared Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flc1125 Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flc1125 commented Nov 11, 2025

Uh oh!

lightsta1ker commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Updating `dice` example based on Updated Specification for the reference application #8099

Updating `dice` example based on Updated Specification for the reference application #8099

flc1125 left a comment •

edited

Loading

pellared Nov 5, 2025 •

edited

Loading

flc1125 Nov 5, 2025 •

edited

Loading