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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 47 commits into from
Jun 21, 2024
Merged

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

merged 47 commits into from
Jun 21, 2024

Conversation

kernelpanic77
Copy link
Contributor

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

@kernelpanic77 kernelpanic77 requested a review from a team February 27, 2024 14:35
@github-actions github-actions bot added connector/count pkg/ottl processor/attributes Attributes processor processor/filter Filter processor processor/routing Routing processor processor/transform Transform processor labels Feb 27, 2024
evan-bradley
evan-bradley reviewed Mar 13, 2024
View reviewed changes
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
Contributor Author

kernelpanic77 commented Mar 19, 2024 •
edited
Loading

@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
Contributor 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
Contributor Author

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

evan-bradley
evan-bradley reviewed Jun 11, 2024
View reviewed changes
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
@kernelpanic77
Copy link
Contributor Author

Can we merge @evan-bradley @TylerHelmuth ?

@evan-bradley
Copy link
Contributor

@kernelpanic77 We should be able to merge this soon. Can you take a look at the failing tests?

@kernelpanic77
Copy link
Contributor Author

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

@evan-bradley
Copy link
Contributor

@TylerHelmuth please take a look.

@TylerHelmuth TylerHelmuth merged commit 889cc4c into open-telemetry:main Jun 21, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jun 21, 2024
martin-majlis-s1 pushed a commit to scalyr/opentelemetry-collector-contrib that referenced this pull request Jun 24, 2024
@kernelpanic77 @evan-bradley
@TylerHelmuth @martin-majlis-s1
[pkg/ottl] Add schema_url field to contexts (open-telemetry#31444)
754d433 
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>
open-telemetry#30229

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

@evan-bradley evan-bradley evan-bradley approved these changes

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu bogdandrutu is a code owner

@jpkrohling jpkrohling Awaiting requested review from jpkrohling jpkrohling is a code owner

@djaglowski djaglowski Awaiting requested review from djaglowski djaglowski is a code owner

@boostchicken boostchicken Awaiting requested review from boostchicken

@kentquirk kentquirk Awaiting requested review from kentquirk

@fchikwekwe fchikwekwe Awaiting requested review from fchikwekwe

@mwear mwear Awaiting requested review from mwear mwear is a code owner

Assignees

@djaglowski djaglowski

Labels
connector/count connector/routing exporter/honeycombmarker internal/filter pkg/ottl processor/attributes Attributes processor processor/filter Filter processor processor/routing Routing processor processor/span processor/tailsampling Tail sampling processor processor/transform Transform processor
Projects
None yet
Milestone
v0.104.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kernelpanic77 @evan-bradley @TylerHelmuth @djaglowski