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] event_index in ottlspanevent TransformContext #35778

Open
michaelsafyan opened this issue Oct 14, 2024 · 5 comments · May be fixed by #37092
Open

[pkg/ottl] event_index in ottlspanevent TransformContext #35778

michaelsafyan opened this issue Oct 14, 2024 · 5 comments · May be fixed by #37092
Labels
enhancement New feature or request pkg/ottl priority:p3 Lowest

Comments

@michaelsafyan
Copy link
Contributor

Component(s)

pkg/ottl

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

I'd like to be able to use the event index in OTTL expressions.

To provide some more background, I'm trying to reuse OTTL for deriving target URIs for uploading data to blob storage (GCS, S3, Azure Blob, or elsewhere). See blobuploadconnector prototype and related issue #33737

You can see here that I need some hackery to support event_index:

func (tracesImpl *tracesToTracesImpl) interpolateSpanEvent(
	ctx context.Context,
	pattern string,
	se *spanEventReference) (string, error) {
  // TODO: eliminate this rewriting when OTTL context for span event supports all of the required fields.
  updatedPattern := strings.ReplaceAll(pattern, "${event_index}", fmt.Sprintf("%v", se.index))
  return tracesImpl.interpolateSpanEventWithOttl(ctx, updatedPattern, se)
}

https://github.com/michaelsafyan/open-telemetry.opentelemetry-collector-contrib/blob/534d8a5dac1618001d3d3c25210fda3a03a39eff/connector/blobuploadconnector/traces.go#L379

The reason that an event index is needed is that there can be more than one event with the same name inside of a given trace span, and so the event index (in combination of the trace ID + span ID) is needed to derive a truly unique URI.

There may be other ways to derive a unique URI (like using the OTTL UUID function). However, being able to substitute in the event_index would be handy for being able to derive a more stable, predictable destination URI.

Describe the solution you'd like

Ideally, the Span Event context would know its index in the containing span and provide an event_index path.

Describe alternatives you've considered

Use functions like UUID to generate unique URIs. This, however, will lead to less predictable URIs as well as make for a less testable solution.

Additional context

No response
@michaelsafyan michaelsafyan added enhancement New feature or request needs triage New item requiring triage labels Oct 14, 2024
@github-actions GitHub Actions
Copy link
Contributor

Pinging code owners:

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

@github-actions github-actions bot mentioned this issue Oct 15, 2024
Closed
@TylerHelmuth TylerHelmuth added discussion needed Community discussion needed and removed needs triage New item requiring triage labels Oct 18, 2024
@TylerHelmuth
Copy link
Member

I think this could be achieved for any of our contexts by adding a new field when creating the TransformContext. I want to think more if it leaks abstraction tho. Typically a "lower" context can know about its "higher" contexts (like span, scope, and resource), but not about its "siblings". We do this to ensure that a function doesn't change more than 1 thing.

@michaelsafyan
Copy link
Contributor Author

Thanks, @TylerHelmuth .

Pardon me, but I'm fairly new to this repo (and most of my experience is closed source)...

What is the process for this discussion / consideration? What are the next steps?

I'd like to also better understand:

We do this to ensure that a function doesn't change more than 1 thing.

Is the concern that the index might be mutated, thereby reordering siblings?

To be clear, I am not suggesting that this be a mutable property. Conceptually, I see the index as an immutable part of the identity of the event (conceptually a part of the "lower" context). I would suggest that, if exposed, that it be immutable.

Or am I misunderstanding the concern?

Thanks for your time!

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Nov 15, 2024 •
edited
Loading

Thought about this some more, I think in NewTransformContext the index can be calculated and stored as a private field on the struct and then a path could be made to access it. It should not be a settable field.

@TylerHelmuth TylerHelmuth added priority:p3 Lowest and removed discussion needed Community discussion needed labels Nov 15, 2024
@TylerHelmuth TylerHelmuth changed the title Feature request: event_index in ottlspanevent TransformContext [pkg/ottl] event_index in ottlspanevent TransformContext Nov 15, 2024
@bacherfl
Copy link
Contributor

bacherfl commented Jan 8, 2025 •
edited
Loading

I would like to look into implementing this @TylerHelmuth - will post a draft PR as soon as I have something

@bacherfl bacherfl linked a pull request Jan 8, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request pkg/ottl priority:p3 Lowest
Projects
None yet
Milestone
No milestone
3 participants
@michaelsafyan @bacherfl @TylerHelmuth