opentelemetry-collector-contrib
opentelemetry-collector-contrib copied to clipboard
open-telemetry
[pkg/ottl] Add schema_url field to contexts
This PR attempts to make schemaURL for TransformContexts.
Description: <Describe what has changed.> Added a breaking change to creation of TransformContext function for logs. Also made changes to all references of the function.
Link to tracking Issue: <Issue number if applicable> #30229
@evan-bradley I think this implementation should now work for ottllog. I have addressed your comments in the latest commits. Requesting your review on this.
However, I still have one question.
how would we change the NewTransformContext function for ottlscope, ottlresource and ottldatapoint, since all these modules use either internal.ResourceContext or internal.InstrumentationScopeContext interface ?
(We cannot use scope/resourceLogs, scope/resourceMetrics, scope/resourceSpans here)
@evan-bradley Lets get this PR merged this week. I think we are very close to what we need. Please give your review for the latest commit. Thanks
@evan-bradley @TylerHelmuth can we merge this PR now ? let me know if any other changes are needed.
Sorry for the delay @kernelpanic77, I have been out for the past two weeks.
Sorry for the delay @kernelpanic77, I have been out for the past two weeks.
No issues @evan-bradley. I have added some of my questions regarding tests in the previous thread. Let me know what you think about it.
@kernelpanic77 Can you verify that you have pushed all changes in your local branch? It looks like the diff hasn't changed.
@kernelpanic77 Can you verify that you have pushed all changes in your local branch? It looks like the diff hasn't changed.
@evan-bradley Yes, I think I forgot to push my local changes. Could you have a look ? I want to discuss an issue I am facing while implementing tests in scope_test.go, I have added comments for the same.
@evan-bradley @TylerHelmuth Does this PR require any more changes ? I think it ready to be merged. Let me know what you think.
@kernelpanic77 looks like the linter is still failing. You can run make lint within the pkg/ottl dir to test if lint is passing.
@kernelpanic77 looks like the linter is still failing. You can run
make lintwithin thepkg/ottldir to test if lint is passing.
@TylerHelmuth the linter is expected to fail here, since the SchemaURLItem interface uses the same signature to get and set a schema_url string.
Error: ottl/contexts/internal/scope_test.go:414:29: var-naming: method SchemaUrl should be SchemaURL (revive) func (t *TestSchemaURLItem) SchemaUrl() string { ^ Error: ottl/contexts/internal/resource.go:95:16: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive) Getter: func(ctx context.Context, tCtx K) (any, error) { ^ Error: ottl/contexts/internal/scope_test.go:418:29: var-naming: method SetSchemaUrl should be SetSchemaURL (revive) func (t *TestSchemaURLItem) SetSchemaUrl(v string) { ^ Error: ottl/contexts/internal/resource_test.go:48:19: unused-parameter: parameter 'resource' seems to be unused, consider removing or renaming it as _ (revive) modified: func(resource pcommon.Resource) { ^ Error: ottl/contexts/internal/resource_test.go:387:2: var-naming: don't use underscores in Go names; struct field schema_url should be schemaURL (revive) schema_url string ^ Error: ottl/contexts/internal/resource_test.go:390:37: var-naming: method SchemaUrl should be SchemaURL (revive) func (t *TestResourceSchemaURLItem) SchemaUrl() string { ^ Error: ottl/contexts/internal/resource_test.go:394:37: var-naming: method SetSchemaUrl should be SetSchemaURL (revive) func (t *TestResourceSchemaURLItem) SetSchemaUrl(v string) {
For instance plog.Resourcelogs also has the same signature as SchemaURLItem interface, to fetch schema_url SchemaUrl()
@kernelpanic77 We should be able to merge this soon. Can you take a look at the failing tests?
@evan-bradley I have fixed most of the integration-tests and unit-tests which seemed to be failing due to my changes. For the listing tests I think we can ignore. Can we please proceed with the merge now ?