opentelemetry-collector-contrib icon indicating copy to clipboard operation
opentelemetry-collector-contrib copied to clipboard

Feature request: `event_index` in `ottlspanevent` TransformContext

Open michaelsafyan opened this issue 1 year ago • 2 comments

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 https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/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 avatar Oct 14 '24 20:10 michaelsafyan

Pinging code owners:

  • pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley

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

github-actions[bot] avatar Oct 14 '24 20:10 github-actions[bot]

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.

TylerHelmuth avatar Oct 18 '24 19:10 TylerHelmuth

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!

michaelsafyan avatar Oct 22 '24 15:10 michaelsafyan

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 avatar Nov 15 '24 20:11 TylerHelmuth

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

bacherfl avatar Jan 08 '25 08:01 bacherfl