[pkg/ottl] Add SpanEvent context #14907
Conversation
|
I don't think "span event" needs to be a context... Do you have any use-case in mind? |
@bogdandrutu filtering SpanEvents is my current goal. But in general, if someone wants to modify SpanEvents with OTTL it'll need a context. |
If only one use-case for the moment, can we find a workaround? |
It might be possible to add a function that would allow users to pass in the names or attributes that the should be dropped, but I think it would be messier, and not as functional, as a context. The traces context provides no access to individual span values, so users could not do If you want, I can build out what I want |
There was a problem hiding this comment.
Looks good to me from a code perspective, just a few nitpicks.
I think this makes sense as a separate context, as there is no map function or looping construct in the OTTL that would permit transformations on a slice of span events within another context. Even if we could loop over slice values, this should simplify things when doing transformations focused on span events.
I think this is the key value that this contexts adds. There are situations where individual manipulation of SpanEvents will be necessary, and a context provides a user-friendly syntax that allows reusing all the ottl's existing functions and its condition framework. We will also run into similar needs with SpanLinks, Examplars, and QuartileValues. Each of these objects, like SpanEvents, cannot currently be manipulated. I think the context concept built into the OTTL provides the perfect mechanism for gaining access to these objects for transformation. |
There was a problem hiding this comment.
Tyler and I reviewed this on a zoom call:
- Someday we'd like to refactor the big switch statement in the *PathGetSetter() function(s) but not as part of this.
- Similarly, I would like to think harder about trying to reduce the redundancy in the StandardGetSetter functions if possible, but it's a separate refactor.
- Much of this PR is just moving code around so it looks bigger than it is.
- This is step 1 of a multi-stage collection of changes.
In terms of "do we need a new context here" I think the answer is definitely yes. A context is the fundamental tool that we use to access elements of the data in OTTL, and we should be creating all the contexts we need to do everything. There is work ongoing to reduce the size and complexity of contexts by making them more general and resuable, and that's the direction we should continue.
|
@bogdandrutu please review |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package ottlspanevents // import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/contexts/ottlspanevents" |
There was a problem hiding this comment.
Should we use ottlspanevent instead? True for other places like the datapoint?
There was a problem hiding this comment.
Ya we aren't consistent. I agree singular is better. I will get this PR updated and then update other contexts in another PR. Also, I think we should rename ottltraces to ottlspan.
TylerHelmuth
added
the
ready to merge
* Add SpanEvent context * add changelog entry and fix lint * ran make goporto * Apply feedback * Fix test * Share span symbol table * fix lint * Rename to ottlspanevent
Description:
Adds a new context specific to Span Events to allow for specific and efficient access to Span Event telemetry.
Link to tracking Issue:
#14578
Testing:
Added unit tests
Documentation:
Added context documentation