AB#486991 open telemetry #34

acofer · 2025-12-09T22:46:30Z

https://dev.azure.com/nintex/Nintex/_workitems/edit/486991

This will be the basis for v1.0.23. Clortho PR: https://github.com/skuid/clortho/pull/79

Copilot

Pull request overview

This pull request adds OpenTelemetry instrumentation to the codebase as an alternative/complementary telemetry solution alongside the existing DataDog StatsD integration. The implementation introduces support for OpenTelemetry traces, metrics, and logs, enabling observability through OTLP exporters and optional Prometheus metrics.

Key Changes

Added comprehensive OpenTelemetry SDK setup with support for traces, metrics, and logs via OTLP exporters
Refactored shared logging structures (logMsg, iso8601) from statsd.go to middlewares.go for reuse across telemetry implementations
Extended metrics monitoring to emit both DataDog and OpenTelemetry metrics simultaneously

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
middlewares/otel.go	New file implementing OpenTelemetry setup, log writer, and event logger integration with zap
middlewares/statsd.go	Removed `logMsg` struct and related code, now consolidated in middlewares.go
middlewares/middlewares.go	Added shared `logMsg` struct and `iso8601` constant moved from statsd.go, plus `encoding/json` import
middlewares/metrics.go	Extended `monitor()` function to emit OpenTelemetry metrics alongside DataDog metrics
go.mod	Added OpenTelemetry dependencies and updated Go version/toolchain specifications
go.sum	Updated checksums for new and updated dependencies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-09T22:52:10Z

middlewares/metrics.go

+	if err != nil {
+		return
+	}
+	ctx := context.Background()


Using context.Background() here loses any context propagation from the HTTP request. Consider passing the request context (e.g., from r.Context()) through to this function to maintain context propagation for tracing and cancellation.

We don't have the context for now.

middlewares/otel.go

Copilot · 2025-12-09T22:52:10Z

middlewares/otel.go

+func (o OpenTelemetryWriter) Write(p []byte) (n int, err error) {
+	var msg logMsg
+	if err = json.Unmarshal(p, &msg); err != nil {
+		return
+	}
+	n = len(p)
+
+	var timestamp time.Time
+	timestamp, err = time.Parse(iso8601, msg.Timestamp)
+	if err != nil {
+		return
+	}
+
+	tags := msg.Tags
+	if tags == nil {
+		tags = []string{}
+	}
+	tagValues := make([]log.Value, len(tags))
+	for i, v := range tags {
+		tagValues[i] = log.StringValue(v)
+	}
+	tagValue := log.SliceValue(tagValues...)
+
+	severity := log.SeverityDebug
+	severityText := "debug"
+	if sm, ok := severityMap[msg.Level]; ok {
+		severity = sm
+		severityText = msg.Level
+	}
+
+	record := log.Record{}
+	record.SetTimestamp(timestamp)
+	record.SetEventName(msg.Name)
+	record.SetSeverity(severity)
+	record.SetSeverityText(severityText)
+	record.AddAttributes(log.KeyValue{
+		Key:   "tags",
+		Value: tagValue,
+	})
+	record.SetBody(log.StringValue(msg.Text()))
+
+	o.logger.Emit(o.ctx, record)
+	return
+}


The error handling in the Write method is incorrect. When an error occurs (lines 155-156 or 161-163), the function returns with n = 0 initially, but if the error occurs after line 158, n is already set to len(p). This violates the io.Writer contract which states that if an error is returned, the number of bytes written should accurately reflect what was successfully written (typically 0 for a failed write). The n value should only be set at the end if the write is successful.

It is not a big problem but this is true, We should follow this pattern

middlewares/otel.go

Copilot · 2025-12-09T22:52:11Z

middlewares/metrics.go

+	oCount, err := oMeter.Int64Counter("http_request_count")
+	if err != nil {
+		return
+	}
+	oDuration, err := oMeter.Float64Histogram("http_request_duration")
+	if err != nil {
+		return
+	}
+	oStatusCount, err := oMeter.Int64Counter(fmt.Sprintf("http_request_status_%s", statusType(httpCode)))
+	if err != nil {
+		return
+	}


Creating new metric instruments (Counter/Histogram) on every request is inefficient. These instruments should be created once at initialization time and reused. Consider creating these instruments in an init function or during setup, and storing them as package-level variables or in a struct.

Thank you for your work on this! I noticed that in the official OpenTelemetry example, variables like oCount, oDuration, and oStatusCount are created once at startup, since they are not global. While the end result is the same, creating these instruments on every request could be inefficient.

I think We could store the instruments as package-level variables, and for dynamic counters like http_request_status_%s, use a map to create them only if they don’t already exist. What do you think about this approach? Let me know if there’s a specific reason for the current implementation!

Yeah, I was thinking about this last night, and we probably need a global map of counters etc. Will get this done this afternoon.

middlewares/metrics.go

middlewares/otel.go

middlewares/metrics.go

gzapatas · 2025-12-10T20:58:01Z

middlewares/metrics.go

+	oCount, err := oMeter.Int64Counter("http_request_count")
+	if err != nil {
+		return
+	}
+	oDuration, err := oMeter.Float64Histogram("http_request_duration")
+	if err != nil {
+		return
+	}
+	oStatusCount, err := oMeter.Int64Counter(fmt.Sprintf("http_request_status_%s", statusType(httpCode)))
+	if err != nil {
+		return
+	}


Thank you for your work on this! I noticed that in the official OpenTelemetry example, variables like oCount, oDuration, and oStatusCount are created once at startup, since they are not global. While the end result is the same, creating these instruments on every request could be inefficient.

I think We could store the instruments as package-level variables, and for dynamic counters like http_request_status_%s, use a map to create them only if they don’t already exist. What do you think about this approach? Let me know if there’s a specific reason for the current implementation!

gzapatas · 2025-12-10T20:58:22Z

middlewares/metrics.go

+	if err != nil {
+		return
+	}
+	ctx := context.Background()


We don't have the context for now.

gzapatas · 2025-12-10T21:03:10Z

middlewares/otel.go

+func (o OpenTelemetryWriter) Write(p []byte) (n int, err error) {
+	var msg logMsg
+	if err = json.Unmarshal(p, &msg); err != nil {
+		return
+	}
+	n = len(p)
+
+	var timestamp time.Time
+	timestamp, err = time.Parse(iso8601, msg.Timestamp)
+	if err != nil {
+		return
+	}
+
+	tags := msg.Tags
+	if tags == nil {
+		tags = []string{}
+	}
+	tagValues := make([]log.Value, len(tags))
+	for i, v := range tags {
+		tagValues[i] = log.StringValue(v)
+	}
+	tagValue := log.SliceValue(tagValues...)
+
+	severity := log.SeverityDebug
+	severityText := "debug"
+	if sm, ok := severityMap[msg.Level]; ok {
+		severity = sm
+		severityText = msg.Level
+	}
+
+	record := log.Record{}
+	record.SetTimestamp(timestamp)
+	record.SetEventName(msg.Name)
+	record.SetSeverity(severity)
+	record.SetSeverityText(severityText)
+	record.AddAttributes(log.KeyValue{
+		Key:   "tags",
+		Value: tagValue,
+	})
+	record.SetBody(log.StringValue(msg.Text()))
+
+	o.logger.Emit(o.ctx, record)
+	return
+}


It is not a big problem but this is true, We should follow this pattern

gzapatas · 2025-12-10T21:04:35Z

go.mod

-go 1.24
+go 1.24.0
+
+toolchain go1.24.4


It is not common on our projects to have this, I usually avoid to do this, is this expected?

No, I'm pretty sure GoLand "helpfully" added this for me. I'll revert it.

This is apparently a new thing as of go 1.21 - just running go build or go mod tidy from the command line insists on adding it. 😖 I think we have to live with it. cli/cli#9489

gzapatas

LGTM!

acofer added 4 commits December 8, 2025 16:13

AB#475100: opentelemetry setup POC

73426d4

Progress commit

7f9e10d

Progress commit

6fccff3

Fixing go build

f6a7f70

acofer requested review from a team and Copilot December 9, 2025 22:46

Copilot started reviewing on behalf of acofer December 9, 2025 22:46 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

gzapatas reviewed Dec 10, 2025

View reviewed changes

middlewares/metrics.go Outdated Show resolved Hide resolved

gzapatas requested changes Dec 10, 2025

View reviewed changes

Address PR comments

9df8f3f

gzapatas self-requested a review December 12, 2025 14:11

gzapatas approved these changes Dec 12, 2025

View reviewed changes

acofer merged commit 5ea5089 into master Dec 12, 2025
1 check passed

AB#486991 open telemetry #34

AB#486991 open telemetry #34

Uh oh!

Conversation

acofer commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 9, 2025

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!

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!

acofer Dec 11, 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!

gzapatas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

acofer commented Dec 9, 2025 •

edited

Loading

acofer Dec 11, 2025 •

edited

Loading