-
Notifications
You must be signed in to change notification settings - Fork 476
WIP: deps update agent #4844
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?
WIP: deps update agent #4844
Conversation
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.
Pull Request Overview
This PR updates major OpenTelemetry Collector dependencies from v1.44.0/v0.138.0 to v1.45.0/v0.139.0, along with related dependency updates. The changes adapt Alloy's codebase to significant API changes in the OTel Collector, particularly around headers configuration (map to list) and telemetry factory initialization.
Key changes:
- OpenTelemetry Collector Core and Contrib packages updated to v0.139.0
- Headers API changed from
map[string]configopaque.Stringtoconfigopaque.MapList(slice of Pair structs) - Telemetry factory initialization added via
otelconftelemetry.NewFactory() - Resource field added to connector TelemetrySettings
- Traces telemetry configuration removed (upstream removal in v0.139.0)
- Multiple dependency updates including Prometheus, Beyla, AWS SDK, and others
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Major dependency updates with new replace directives for OTel packages using specific commits; includes detailed documentation file |
| internal/static/traces/config.go | Adds telemetry factory to OTel collector factories initialization |
| internal/converter/internal/otelcolconvert/converter_service_telemetry.go | Updates telemetry converter to handle new API with type assertion; removes traces conversion |
| internal/converter/internal/otelcolconvert/converter_otlpexporter.go | Converts headers from map to MapList format |
| internal/component/otelcol/extension/jaeger_remote_sampling/ | Adapts headers handling for new MapList API |
| internal/component/otelcol/exporter/*/test.go | Updates test expectations for new queue config structure and headers format |
| internal/component/otelcol/connector/connector.go | Adds Resource initialization for telemetry settings |
| internal/component/otelcol/config_http.go | Converts headers map to MapList in HTTP client config |
| internal/component/otelcol/config_grpc.go | Converts headers map to MapList in gRPC client config |
| internal/component/loki/source/syslog/.../syslogparser_test.go | Updates error message expectation for upstream library change |
| internal/component/database_observability/lexer_test.go | Adjusts test for changed SQL lexer behavior (no longer errors on unclosed quotes) |
| deps-update-2025-11-13.md | Comprehensive documentation of the dependency update process and decisions |
| .cursor/rules/major-dep-updates.md | New documentation file with guidelines for future major dependency updates |
|
|
||
| // Initialize Resource for TelemetrySettings. The OpenTelemetry Collector service | ||
| // automatically sets this with service.instance.id, but in Alloy we need to initialize | ||
| // it ourselves. The connector code will handle generating an instance ID if needed. | ||
| resource := pcommon.NewResource() |
Copilot
AI
Nov 14, 2025
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.
[nitpick] The Resource field is initialized with an empty resource (pcommon.NewResource()). According to the comment on lines 169-171, the OpenTelemetry Collector service automatically sets this with service.instance.id, but in Alloy it needs manual initialization. However, without setting any attributes on the empty resource, this may not provide the expected telemetry context. Consider documenting what attributes (if any) should be set on this resource, or if the connector code truly handles generating the instance ID automatically as mentioned in the comment.
| require.NoError(t, err) | ||
| // The query doesn't contain reserved keywords, so result should be false | ||
| require.False(t, result) | ||
| }) |
Copilot
AI
Nov 14, 2025
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.
The comment states "Note: The SQL lexer library has been updated and no longer returns errors for unclosed quotes." This is a significant behavioral change. The test now expects no error where previously an error was expected. Consider:
- Is this change in the upstream library intentional or a regression?
- Should we add a test case that validates the function still correctly identifies reserved keywords even with unclosed quotes?
- The security implications - does silently handling malformed SQL queries pose any risks?
The change appears to be from updating github.com/DataDog/go-sqllexer from v0.1.6 to v0.1.9.
| }) | |
| }) | |
| t.Run("reserved keyword in unclosed quote", func(t *testing.T) { | |
| // This test checks that reserved keyword detection works even if the query is malformed (unclosed quote) | |
| // The reserved keyword "SELECT" is present inside an unclosed quote | |
| result, err := ContainsReservedKeywords("SELECT \"SELECT", ExplainReservedWordDenyList, sqllexer.DBMSMySQL) | |
| require.NoError(t, err) | |
| // The query contains a reserved keyword, so result should be true | |
| require.True(t, result) | |
| }) |
| // Updated slim proto packages to v0.2.0 to match pprofile v0.139.0 requirements (upstream uses v0.2.0) | ||
| // These packages are required by pprofile v0.139.0 | ||
| replace ( | ||
| go.opentelemetry.io/proto/slim/otlp/collector/profiles/v1development => go.opentelemetry.io/proto/slim/otlp/collector/profiles/v1development v0.0.1 | ||
| go.opentelemetry.io/proto/slim/otlp/profiles/v1development => go.opentelemetry.io/proto/slim/otlp/profiles/v1development v0.0.1 | ||
| go.opentelemetry.io/proto/slim/otlp/collector/profiles/v1development => go.opentelemetry.io/proto/slim/otlp/collector/profiles/v1development v0.2.0 | ||
| go.opentelemetry.io/proto/slim/otlp/profiles/v1development => go.opentelemetry.io/proto/slim/otlp/profiles/v1development v0.2.0 | ||
| ) |
Copilot
AI
Nov 14, 2025
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.
[nitpick] The replace directives for slim proto packages were updated from v0.0.1 to v0.2.0 with improved comments explaining they're required by pprofile v0.139.0. However, the comment on line 1106 says "Updated slim proto packages to v0.2.0" but the actual replace directives still appear to be present. Consider verifying:
- Are these replace directives still necessary with the new versions?
- The upstream comment mentions these are needed for pprofile v0.139.0 - is this a temporary workaround or permanent requirement?
- Can these be removed once all dependencies align on the same versions?
| require github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter v0.0.0-00010101000000-000000000000 | ||
|
|
Copilot
AI
Nov 14, 2025
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.
Line 1003 adds a new require statement for the loadbalancing exporter with a zero timestamp version, which is then replaced on line 1127 with a specific commit. This pattern seems unusual - typically you'd either:
- Use the replace directive only (without the require)
- Or use a valid version in the require and override with replace
The current approach with v0.0.0-00010101000000-000000000000 may cause confusion. Consider using a proper version tag or commit pseudo-version directly in the require statement, or remove the explicit require if the replace directive is sufficient.
| require github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter v0.0.0-00010101000000-000000000000 |
| // Type assert to otelconftelemetry.Config | ||
| otelTel, ok := tel.(otelconftelemetry.Config) | ||
| if !ok { | ||
| // If it's not the expected type, skip conversion |
Copilot
AI
Nov 14, 2025
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.
The function signature changed from accepting otelconftelemetry.Config to component.Config, which is more generic. However, the type assertion approach adds runtime failure risk. Consider adding error diagnostics when the type assertion fails to help users understand why their telemetry configuration was skipped.
Suggested enhancement:
if !ok {
diags.Add(diag.SeverityLevelWarn, "service telemetry configuration has unexpected type, skipping conversion")
return diags
}| // If it's not the expected type, skip conversion | |
| diags.Add(diag.SeverityLevelWarn, "service telemetry configuration has unexpected type, skipping conversion") |
| func toHeadersMap(cfg configopaque.MapList) map[string]string { | ||
| res := make(map[string]string, len(cfg)) | ||
| for k, v := range cfg { | ||
| res[k] = string(v) | ||
| for _, pair := range cfg { | ||
| res[pair.Name] = string(pair.Value) | ||
| } |
Copilot
AI
Nov 14, 2025
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.
[nitpick] The conversion from map[string]configopaque.String to configopaque.MapList changes the order guarantees. Maps in Go have undefined iteration order, but appending to a slice preserves insertion order. While this is likely intentional and aligns with the upstream API change, be aware that headers may now be sent in a consistent order, which could affect behavior if any systems were relying on specific header ordering (though this is unlikely).
| defaultQueueConfig = exporterhelper.QueueBatchConfig{ | ||
| Enabled: true, | ||
| NumConsumers: 10, | ||
| QueueSize: 1000, | ||
| BlockOnOverflow: false, | ||
| Sizer: exporterhelper.RequestSizerTypeRequests, | ||
| WaitForResult: false, | ||
| Batch: configoptional.None[exporterhelper.BatchConfig](), | ||
| } |
Copilot
AI
Nov 14, 2025
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.
The defaultQueueConfig is now explicitly constructed instead of using exporterhelper.NewDefaultQueueConfig(). This change appears in multiple test files. While this provides more visibility into the actual default values, it creates maintenance burden if the upstream defaults change - these tests would need manual updates. Consider:
- Checking if
exporterhelper.NewDefaultQueueConfig()still exists and why it can't be used - If it was removed upstream, document this in a comment
- Creating a helper function in the test package to generate default configs to avoid duplication across multiple test files
| // The fix was merged in PR #43960 (merge commit 5aa2746117ecd5905bd8e7df60a3e5ca1465bbd7) but hasn't been released in a tagged version yet. | ||
| // Using the merge commit SHA - go mod tidy will convert it to the proper pseudo-version. | ||
| replace github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter => github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter v0.139.1-0.20251106155258-5aa2746117ec |
Copilot
AI
Nov 14, 2025
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.
Multiple replace directives reference specific commit SHAs and pseudo-versions (e.g., v0.139.1-0.20251105131800-09a271914bdc). These are using unreleased commits from the main branch. While this is acceptable for WIP, before merging:
- Verify these commits are stable and don't introduce breaking changes
- Check if newer official releases have been published that include these fixes
- Document why these specific commits are needed (the comment on line 1124-1126 explains the loadbalancing exporter fix, which is good)
- Consider the maintenance burden - tracking specific commits makes future updates more complex
| // The fix was merged in PR #43960 (merge commit 5aa2746117ecd5905bd8e7df60a3e5ca1465bbd7) but hasn't been released in a tagged version yet. | |
| // Using the merge commit SHA - go mod tidy will convert it to the proper pseudo-version. | |
| replace github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter => github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter v0.139.1-0.20251106155258-5aa2746117ec | |
| // The fix is now included in the official release v0.140.0. | |
| replace github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter => github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter v0.140.0 |
|
|
||
| TracerProvider: p.opts.Tracer, | ||
| MeterProvider: mp, | ||
| Resource: resource, |
Copilot
AI
Nov 14, 2025
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.
The new Resource field initialization in the TelemetrySettings (line 181) is not covered by any visible test updates. Consider adding a test case that verifies:
- The Resource is properly initialized (not nil)
- The connector properly handles or generates any required resource attributes
- The Resource propagates correctly through the telemetry pipeline
This ensures the change doesn't introduce runtime issues where code expects the Resource to have specific attributes.
| diags.AddAll(convertMetrics(file, tel.Metrics)) | ||
| diags.AddAll(convertTraces(file, tel.Traces)) | ||
| // Type assert to otelconftelemetry.Config | ||
| otelTel, ok := tel.(otelconftelemetry.Config) |
Copilot
AI
Nov 14, 2025
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.
The type assertion on line 17 uses otelconftelemetry.Config but should use *otelconftelemetry.Config (pointer type). The component.Config interface is typically implemented by pointer types, and the conversion will likely fail at runtime with the current value receiver type.
Suggested fix:
otelTel, ok := tel.(*otelconftelemetry.Config)
PR Description
Which issue(s) this PR fixes
Notes to the Reviewer
PR Checklist