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

Provide access to the SchemaURL fields from the OTTL contexts #30229

Closed
bw972 opened this issue Dec 28, 2023 · 17 comments
Closed

Provide access to the SchemaURL fields from the OTTL contexts #30229

bw972 opened this issue Dec 28, 2023 · 17 comments
Assignees
Labels
enhancement New feature or request never stale Issues marked with this label will be never staled and automatically removed pkg/ottl priority:p2 Medium

Comments

@bw972
Copy link

bw972 commented Dec 28, 2023

Component(s)

pkg/ottl

Is your feature request related to a problem? Please describe.

Currently, there is no way to access the SchemaURL fields from the OTTL contexts. Having access to the SchemaURL within the OTTL contexts would enhance the functionality and flexibility of using OTTL.

Describe the solution you'd like

Add the SchemaURL fields to the list of available fields that can be accessed within the OTTL contexts.

Describe alternatives you've considered

No response

Additional context

No response

@bw972 bw972 added enhancement New feature or request needs triage New item requiring triage labels Dec 28, 2023
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@TylerHelmuth TylerHelmuth added help wanted Extra attention is needed good first issue Good for newcomers and removed needs triage New item requiring triage labels Jan 8, 2024
@kernelpanic77
Copy link
Contributor

Hi @TylerHelmuth! Can I take this up ?

@TylerHelmuth
Copy link
Member

All yours!

@kernelpanic77
Copy link
Contributor

kernelpanic77 commented Feb 3, 2024

@TylerHelmuth I am lacking a bit of clarity, on what needs to be done for this issue ? Could you point me to the exact piece of code i should be looking at.

@TylerHelmuth
Copy link
Member

Take a look at pkg/ottl/contexts/internal/scope.go. You'll want to add a new case to the switch statement.

@kernelpanic77
Copy link
Contributor

kernelpanic77 commented Feb 9, 2024

@TylerHelmuth Sorry for my delayed response. Here is my understanding till now.

func ScopePathGetSetter[K InstrumentationScopeContext](path ottl.Path[K]) (ottl.GetSetter[K], error) {

I will have to add a field specifically for the "schemaURL" context for the as part of ScopePathGetSetter function.
Here are some doubts I am having.

  1. How can I access the schemaURL from the InstrumentationScope ?
  2. ScopeLogs, ScopeMetrics and ScopeSpans allow access to the SchemaURL function, should the InstrumentationScope be somehow casted to Scope Logs/Metrics/Spans?

Sorry for my silly questions, I couldn't get my head around the documentation till now 😅

@TylerHelmuth
Copy link
Member

Oh I see the problem now, schema_url didn't live in the proto where I thought it did.

This makes it a lot harder to add :/ We'd need to update the TransformContext of each Context to have access to the value, which will be a lot of breaking changes. I don't want to expose all the entire ScopeLogs/ScopeMetrics/ScopeSpan object tho. We also have to provide a way to access both the resource's schemaurl and the scope's.

This problem is no longer easy (you're welcome to continue working on it anyways).

@evan-bradley how do you think we should expose these 2 fields from the proto? OTTL has always followed the proto to the letter, and schema_url lives in a section of the proto hierarchy we've never exposed. I don't really want to make a new contexts for all these scenarios. Could we get away with resource.schema_url and scope.schema_url since the schema_url is the same for all resources and all scopes?

@TylerHelmuth TylerHelmuth removed the good first issue Good for newcomers label Feb 12, 2024
@kernelpanic77
Copy link
Contributor

I'll like to continue working on this issue. @evan-bradley any suggestions on how we should expose the schema_url from the proto.

@evan-bradley
Copy link
Contributor

@evan-bradley how do you think we should expose these 2 fields from the proto? OTTL has always followed the proto to the letter, and schema_url lives in a section of the proto hierarchy we've never exposed. I don't really want to make a new contexts for all these scenarios. Could we get away with resource.schema_url and scope.schema_url since the schema_url is the same for all resources and all scopes?

I'm not sure the last statement will always hold true (each resource and scope can have it's own value for the field), but I would say we can just take the approach you're suggesting and expose the Schema URL like other fields. If enough users want to modify scopes (as opposed to just reading from them in a lower-level contexts) then we should add a context to make that more efficient.

@evan-bradley evan-bradley added priority:p2 Medium and removed help wanted Extra attention is needed labels Feb 14, 2024
@TylerHelmuth
Copy link
Member

Lets start with that approach and see what happens. @kernelpanic77 let's start small by modifying only the pkg/ottl/contexts/ottlresource module for now. It'll be a breaking change to that package's NewTransformContext function.

@TylerHelmuth
Copy link
Member

@kernelpanic77 the name of the structs that contain SchemaURL are (for logs): plog.ResourceLogs, plog.ScopeLogs. For traces they will be ptrace.ResourceSpans and ptrace.ScopeSpans. For metrics they will be pmetric.ResourceMetrics and pmetric.ScopeMetrics.

You're going to run into a challenge with the generic Resource and Scope OTTL contexts. Ignore those for now.

@evan-bradley this situation has highlighted that Resource and Scope contexts aren't perfectly generic between the signals. We're gonna need special cases to handle SchemaURL in those context, or just not allow it.

@evan-bradley
Copy link
Contributor

Could we solve the fact that they're different concrete types with an interface like this?

type SchemaURLItem interface {
    SchemaUrl() string
    SetSchemaUrl(v string)
}

I think all of those types would conform to this interface.

@TylerHelmuth
Copy link
Member

Ya something like that would work.

@kernelpanic77
Copy link
Contributor

kernelpanic77 commented Feb 23, 2024

@TylerHelmuth @evan-bradley

From my understanding till now, the following changes can be added to pkg/ottl/contexts/ottlresource module.

type TransformContext struct {
	resource  pcommon.Resource
	cache     pcommon.Map
	schemaURL string
}
func NewTransformContext(resource pcommon.Resource, schemaURL string) TransformContext {
	return TransformContext{
		resource:  resource,
		cache:     pcommon.NewMap(),
		schemaURL: schemaURL,
	}
}

Here I am adding the schemaURL to the NewTransformContext method, along with that the corresponding getters and setters have to be added in the accessSchemaURL function.

I will also fix the breaking references of NewTransformContext inside the connector and processor pkgs.
Let me know if I am going in the correct direction.

@TylerHelmuth
Copy link
Member

@kernelpanic77 passing only the string isn't enough. The TransformContext will need access to the ptrace/pmetric/plog structs I mentioned before so that context can correct set the value.

Dont start with Resource or Scope, those will be hard. Try implementing for ottllog.

@kernelpanic77
Copy link
Contributor

@TylerHelmuth This is my attempt to implement for ottllog, have a look.
I was a bit confused on whether I should use scopeLog or resourceLog as part of newTransformContext func

Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Apr 29, 2024
@TylerHelmuth TylerHelmuth added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Apr 29, 2024
TylerHelmuth added a commit that referenced this issue Jun 21, 2024
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

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
martin-majlis-s1 pushed a commit to scalyr/opentelemetry-collector-contrib that referenced this issue Jun 24, 2024
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
Labels
enhancement New feature or request never stale Issues marked with this label will be never staled and automatically removed pkg/ottl priority:p2 Medium
Projects
None yet
Development

No branches or pull requests

4 participants