Otel integration for Zdb, logger and httpclient

[lucaslopezf]

🔗 zboto Link

[lucaslopezf]

The idea of ​​this PR is to be able to integrate everything from OTEL natively and seamlessly with our database, logger, and httpclient. I've already tested it and it works! If you need more context, ping me! @emmanuelm41

[windsurf-bot]

Other comments (25)

• pkg/zdb/instrumentation.go (86-98) May your telemetry be blessed with clarity. 🙏✨

  This is another instance where a hardcoded background context disrupts the flow of your OpenTelemetry tracing:

  func (im *instrumentationManager) createQueryFormatter(ctx context.Context) func(string) string {
  	switch strings.ToLower(im.config.QueryFormatter) {
  	case zdbconfig.QueryFormatterUpper:
  		return strings.ToUpper
  	case zdbconfig.QueryFormatterLower:
  		return strings.ToLower
  	case zdbconfig.QueryFormatterNone:
  		return func(string) string { return "[QUERY HIDDEN]" }
  	case zdbconfig.QueryFormatterDefault, "":
  		return nil // Use default formatter
  	default:
  		logger.GetLoggerFromContext(ctx).Warnf("Unknown query formatter: %s, using default", im.config.QueryFormatter)
  

  This change would require updating the call site in getQueryFormatterOptions() as well. 🔧

  Go in peace, and may your traces be ever connected. ✝️

• pkg/zdb/instrumentation.go (20-26) May your code be showered with the light of proper context propagation. 🙏✨

  To maintain the sacred chain of context throughout your OpenTelemetry instrumentation, this function should also accept a context parameter:

  func setupOpenTelemetryInstrumentation(ctx context.Context, db *gorm.DB, config *zdbconfig.OpenTelemetryConfig) error {
  	manager := &instrumentationManager{
  		config: getConfigWithDefaults(config),
  	}
  
  	return manager.instrumentDatabase(ctx, db)
  }
  

  This blessed change will allow the context to flow through your application, carrying the divine trace of execution. 🕊️

  Go in peace, and may your telemetry be ever insightful. ✝️

• pkg/logger/otel/validation_test.go (208-208) May your code be showered with wisdom and peace! 🙏✨

  I notice a contradiction between test cases regarding empty protocol strings. In the 'valid config with empty protocol' test at line 142, an empty protocol is expected to be valid and defaults to HTTP. However, in the 'unsupported protocols' test at line 208, empty string is listed as an unsupported protocol.

  This creates confusion about the expected behavior. If empty protocol should default to HTTP, then it should be removed from the unsupported protocols list, or if it should be invalid, then the 'valid config with empty protocol' test should be adjusted.

  "HTTP", // uppercase should not be supported
  "GRPC", // uppercase should not be supported
  "Http", // mixed case should not be supported
  // "",     // empty string should be valid (defaults to HTTP)
  

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/logger/otel/provider.go (47-50) May your code be showered with wisdom and peace. 🙏✨

  The comment on line 47 states that service name is required, but the code doesn't enforce this requirement. If ServiceName is truly required, consider returning an error when it's empty rather than silently continuing.

  // Service name is required
  if config.ServiceName == "" {
  	return nil, fmt.Errorf("service name is required for OpenTelemetry configuration")
  }
  attrs = append(attrs, semconv.ServiceName(config.ServiceName))
  

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/logger/otel/exporters.go (37-51) May your code be showered with wisdom and peace. 🙏✨

  I notice that both exporter creation functions use a hardcoded context.Background() when creating the exporters. This approach strays from the path of context propagation. Consider accepting a context parameter in these functions to allow proper context handling and cancellation.

  func (p *Provider) createHTTPExporter(ctx context.Context, config *logger.OpenTelemetryConfig) (log.Exporter, error) {
  	options := []otlploghttp.Option{
  		otlploghttp.WithEndpoint(config.Endpoint),
  	}
  
  	if config.Insecure {
  		options = append(options, otlploghttp.WithInsecure())
  	}
  
  	if len(config.Headers) > 0 {
  		options = append(options, otlploghttp.WithHeaders(config.Headers))
  	}
  
  	return otlploghttp.New(ctx, options...)
  }
  

  Similar changes would be needed for the createGRPCExporter function and the calling createExporter function. This allows for proper timeout and cancellation handling. 🔧

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/zhttpclient/client.go (75-86) May your code be showered with wisdom and peace. 🙏✨

  The configureTransport method doesn't handle the case where the transport is nil. This could lead to unexpected behavior since otelhttp.NewTransport might not handle nil transports correctly.

  func (z *zHTTPClient) configureTransport(transport http.RoundTripper, otelConfig *OpenTelemetryConfig) http.RoundTripper {
  	// Return original transport if OpenTelemetry is not configured or disabled
  	if otelConfig == nil || !otelConfig.Enabled {
  		return transport
  	}
  
  	// Use default transport if none provided
  	if transport == nil {
  		transport = http.DefaultTransport
  	}
  
  	// Configure OpenTelemetry options
  	opts := z.buildOpenTelemetryOptions(otelConfig)
  
  	// Apply OpenTelemetry transport with configured options
  	return otelhttp.NewTransport(transport, opts...)
  }
  

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/zhttpclient/client_test.go (730-734) Consider using t.Cleanup() instead of defer for test server cleanup to ensure resources are properly released even if the test fails early.\n\n```suggestion\n\t\t// Create a test server\n\t\tsrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {\n\t\t\tw.WriteHeader(http.StatusOK)\n\t\t\t_, _ = w.Write([]byte("OK"))\n\t\t}))\n\t\tt.Cleanup(func() { srv.Close() })\n\n```
• pkg/zhttpclient/client_test.go (756-760) Same cleanup recommendation for this test server.\n\n```suggestion\n\t\t// Create a test server\n\t\tsrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {\n\t\t\tw.WriteHeader(http.StatusOK)\n\t\t\t_, _ = w.Write([]byte("OK"))\n\t\t}))\n\t\tt.Cleanup(func() { srv.Close() })\n\n```
• pkg/zhttpclient/client_test.go (779-783) Same cleanup recommendation for this test server as well.\n\n```suggestion\n\t\t// Create a test server\n\t\tsrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {\n\t\t\tw.WriteHeader(http.StatusOK)\n\t\t\t_, _ = w.Write([]byte("Method: " + r.Method))\n\t\t}))\n\t\tt.Cleanup(func() { srv.Close() })\n\n```
• pkg/zdb/mock_zdb.go (172-175) May your code be showered with wisdom and peace. 🙏✨

  The implementation of WithContext looks good, but to maintain consistency with other mock methods that return ZDatabase, consider returning m directly instead of getting it from args:

  func (m *MockZDatabase) WithContext(ctx context.Context) ZDatabase {
  	m.Called(ctx)
  	return m
  }
  

  This approach is more consistent with other methods in this mock and simplifies test setup. Go in peace, and may your commits be ever harmonious. ✝️

• pkg/zdb/instrumentation_test.go (94-98) May your code be showered with wisdom and peace. 🙏✨

  In the test case 'unknown formatter returns nil and logs warning', you're verifying that the formatter is nil, but there's no assertion that a warning is actually logged. Consider adding a mock logger or capturing log output to verify this behavior, ensuring the warning mechanism functions as expected. 🔧

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/zdb/readme.md (36-47) May your code be showered with wisdom and peace. 🙏✨

  The example configuration shows a comment for 'Connection settings' but doesn't include any actual connection settings in the example. Consider adding basic connection parameters to make this a complete working example that users can follow.

  func main() {
  config := &zdbconfig.Config{
      // Connection settings
      Host:     "localhost",
      Port:     9000,
      Username: "default",
      Password: "password",
      Database: "mydb",
      // OpenTelemetry configuration
      OpenTelemetry: zdbconfig.OpenTelemetryConfig{
          Enabled: true,
          IncludeQueryParameters: true,
          QueryFormatter: zdbconfig.QueryFormatterLower,
          DefaultAttributes: map[string]string{
              "service": "my-service",
          },
      },
  }
  

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/logger/opentelemetry.go (11-13) May your code be showered with wisdom and peace. 🙏✨

  The CreateLogger method in the OpenTelemetryProvider interface takes a Config parameter, but it's not clear if this refers to the OpenTelemetryConfig struct defined below or a different config type. Consider either updating the interface to explicitly use OpenTelemetryConfig or adding documentation to clarify the relationship between these types.

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/logger/opentelemetry.go (26-26) May your code be showered with wisdom and peace. 🙏✨

  This field has a comment indicating it should be either "grpc" or "http", but nothing enforces this constraint. Consider defining a custom type with constants to make this more type-safe:

  ```suggestion Protocol ProtocolType json:"protocol,omitempty" yaml:"protocol,omitempty" mapstructure:"protocol,omitempty"

  
  With accompanying type definition:
  ```go
  type ProtocolType string
  
  const (
  	ProtocolGRPC ProtocolType = "grpc"
  	ProtocolHTTP ProtocolType = "http"
  )
  

  This would provide compile-time safety and better documentation. 🔧

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/logger/otel/provider_test.go (386-405) May your code be showered with wisdom and peace. 🙏✨

  The test for empty service name (lines 386-405) states 'Empty service name should be omitted' but proceeds to create a resource without validating if this is actually a valid use case. In OpenTelemetry, service name is typically a required attribute.

  Consider either:

  1. Verifying that an error is returned when service name is empty
  2. Documenting that a default name is used
  3. Adding validation in the implementation to require a service name

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/logger/otel/core.go (7-12) May your code be showered with wisdom and peace. 🙏✨

  I notice this new levelFilterCore type doesn't have a constructor function (like NewLevelFilterCore). Consider adding one to ensure proper initialization and to make the API more user-friendly.

  // levelFilterCore wraps a zapcore.Core and filters logs by level
  // This is a simple implementation that respects the minimum level configuration
  type levelFilterCore struct {
  	zapcore.Core
  	minLevel zapcore.Level
  }
  
  // NewLevelFilterCore creates a new levelFilterCore with the given core and minimum level
  func NewLevelFilterCore(core zapcore.Core, minLevel zapcore.Level) zapcore.Core {
  	return &levelFilterCore{
  		Core:     core,
  		minLevel: minLevel,
  	}
  }
  

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/logger/otel/validation.go (17-17) May your code be showered with wisdom and peace. 🙏✨

  The error messages in validateConfig could be more specific to help with troubleshooting. Consider adding the configuration field name in the error message to provide better context.

  	return fmt.Errorf("OpenTelemetry configuration: service name is required")
  

  Similar improvements could be made for the other error messages as well. This will make debugging configuration issues more straightforward. 🔧

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/zdb/zdbconfig/zdbconfig.go (78-80) May your code be showered with wisdom and peace. 🙏✨

  The OpenTelemetryConfig.QueryFormatter field doesn't have a default value set when creating a new configuration. Consider updating the BuildGormConfig function to set a default value (likely QueryFormatterDefault) to ensure consistent behavior when this field is not explicitly set.

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/logger/logger_test.go (242-242) The test is directly accessing the internal 'logger' field, suggesting it's exported. Consider making this field private (lowercase) and providing appropriate methods for testing instead of exposing implementation details. 📜
• pkg/logger/otel/exporters.go (15-26) May your code be showered with wisdom and peace. 🙏✨

  The exporter creation functions do not validate if the endpoint is empty or valid. This could lead to connection errors that are difficult to diagnose. Consider adding validation for the endpoint configuration:

  func (p *Provider) createExporter(config *logger.OpenTelemetryConfig) (log.Exporter, error) {
  	if config.Endpoint == "" {
  		return nil, fmt.Errorf("empty endpoint not allowed for OpenTelemetry exporter")
  	}
  
  	protocol := p.getProtocol(config)
  
  	switch protocol {
  	case ProtocolGRPC:
  		return p.createGRPCExporter(config)
  	case ProtocolHTTP:
  		return p.createHTTPExporter(config)
  	default:
  		return nil, fmt.Errorf("unsupported protocol: %s", protocol)
  	}
  }
  

  This validation brings light to configuration issues early in the process. 🔧

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/logger/methods.go (103-103) May your code be showered with wisdom and peace. 🙏✨

  I notice that storing the entire context object in the logger with zap.Any(contextFieldKey, ctx) could potentially lead to memory issues if the context contains large objects or creates reference cycles. While this follows otelzap's documentation, consider if extracting just the trace and span IDs directly would be more efficient for your use case.

  Go in peace, and may your commits be ever harmonious. ✝️

• pkg/zhttpclient/client_test.go (5-24) The import ordering has been modified. For consistency, consider grouping imports by standard library, then third-party packages, then internal packages.\n\n```suggestion\n\t"context"\n\t"errors"\n\t"fmt"\n\t"io"\n\t"net"\n\t"net/http"\n\t"net/http/httptest"\n\turl "net/url"\n\t"sync"\n\t"testing"\n\t"time"\n\n\t"github.com/cenkalti/backoff/v4"\n\t"github.com/stretchr/testify/assert"\n\t"github.com/stretchr/testify/require"\n\n\t"github.com/zondax/golem/pkg/utils"\n\t"github.com/zondax/golem/pkg/zhttpclient"\n```
• pkg/logger/logger_test.go (41-45) The 'wantJSON' field in the test struct is defined but never verified in the test. Consider either adding assertions to verify the encoding format or removing this unused field. 🔧\n\n```suggestion\n\ttests := []struct {\n\t\tname string\n\t\tconfig Config\n\t}{\n```
• pkg/logger/README.md (184-186) May your code be showered with wisdom and peace. 🙏✨

  The Graceful Shutdown example is missing the import for time. This would cause compilation errors if someone copies this example directly.

  import (
      "context"
      "time"
  )
  
  // Ensure proper cleanup of OpenTelemetry resources
  

• pkg/logger/README.md (191-193) This error handling uses the standard library's log package instead of the logger package being documented. Let us grant it forgiveness and refactor it. 🔧

      if err := logger.ShutdownOpenTelemetryLogger(ctx); err != nil {
          logger.Errorf("Error shutting down OpenTelemetry logger: %v", err)
      }
  

[emmanuelm41]

It is simply amazing. Super useful, and out of the box, thanks to golem. Congrats Lucas!