Skip to content
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

[pkg/ottl] Add schema_url field to contexts #31444

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

kernelpanic77
Copy link

This PR attempts to make schemaURL for TransformContexts.

Description:
Added a breaking change to creation of TransformContext function for logs. Also made changes to all references of the function.

Link to tracking Issue:
#30229

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience. We're headed in the right direction, I left a few comments to help illustrate how I think this should work.

pkg/ottl/contexts/internal/scope.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/internal/scope.go Show resolved Hide resolved
pkg/ottl/contexts/ottllog/log.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/ottllog/log.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/ottllog/log.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/ottllog/log.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/ottllog/log.go Outdated Show resolved Hide resolved
@kernelpanic77
Copy link
Author

kernelpanic77 commented Mar 19, 2024

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

@kernelpanic77
Copy link
Author

@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

@kernelpanic77
Copy link
Author

@evan-bradley @TylerHelmuth can we merge this PR now ?
let me know if any other changes are needed.

@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Jun 3, 2024
@evan-bradley
Copy link
Contributor

@kernelpanic77 Could you fix the lint issues?

@TylerHelmuth
Copy link
Member

running make fmt in the transformprocessor dir should fix the issue.

@TylerHelmuth TylerHelmuth removed the ready to merge Code review completed; ready to merge by maintainers label Jun 3, 2024
@kernelpanic77
Copy link
Author

Ready to merge 👍

@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Jun 4, 2024
@TylerHelmuth
Copy link
Member

@kernelpanic77 looks like the linter is still failing. You can run make lint within the pkg/ottl dir to test if lint is passing.

@TylerHelmuth TylerHelmuth removed the ready to merge Code review completed; ready to merge by maintainers label Jun 4, 2024
@kernelpanic77
Copy link
Author

kernelpanic77 commented Jun 4, 2024

@kernelpanic77 looks like the linter is still failing. You can run make lint within the pkg/ottl dir 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()

pkg/ottl/contexts/ottlcommon/package_test.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/ottlcommon/README.md Outdated Show resolved Hide resolved
pkg/ottl/contexts/ottlcommon/schema.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants