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

Instrument AddEvent from default OTel default global impl #541

Closed
MrAlias opened this issue Dec 7, 2023 · 8 comments
Closed

Instrument AddEvent from default OTel default global impl #541

MrAlias opened this issue Dec 7, 2023 · 8 comments

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Dec 7, 2023

Originally posted by @MrAlias in #523 (comment)

@asecondo
Copy link
Contributor

i'll pick this up

@asecondo
Copy link
Contributor

@RonFed any recommendations for how to handle the variadic interface input that comes along with span.AddEvent? i have a working implementation for handling the event name and passing that back to the go code, but i'm not sure how to best handle inspecting the underlying implementations of trace.EventOption.

@RonFed
Copy link
Contributor

RonFed commented Jan 29, 2024

I am thinking about attaching to the following functions:

  • func WithTimestamp(t time.Time) SpanEventOption, if this is not used we should use bpf_ktime_get_ns() for event timestamp.
  • func WithAttributes(attributes ...attribute.KeyValue) SpanStartEventOption (I think this one is also relevant to the Start function).

The tricky part is associating these calls with the span we are building in eBPF.
One option that I can think of, is adding a hash map with keys as the goroutine pointer (similar to what get_consistent_key returns), and the value will be a pointer to the concrete option (the return pointer from the above functions). Then in the AdddEvent probe we can lookup these options, apply them and delete them from the map.

@asecondo
Copy link
Contributor

asecondo commented Jan 30, 2024

I am thinking about attaching to the following functions:

  • func WithTimestamp(t time.Time) SpanEventOption, if this is not used we should use bpf_ktime_get_ns() for event timestamp.
  • func WithAttributes(attributes ...attribute.KeyValue) SpanStartEventOption (I think this one is also relevant to the Start function).

The tricky part is associating these calls with the span we are building in eBPF. One option that I can think of, is adding a hash map with keys as the goroutine pointer (similar to what get_consistent_key returns), and the value will be a pointer to the concrete option (the return pointer from the above functions). Then in the AdddEvent probe we can lookup these options, apply them and delete them from the map.

@RonFed nearly have something that works with having a separate probe for func WithAttributes(attributes ...attribute.KeyValue) SpanStartEventOption. i have an e2e test that works, but have a hard coded loop value that otherwise results in a max value being exceeded in a register for some reason. should hopefully figure that out soon. i also needed to build the go e2e test binary with inlining disabled because it turns out WithAttributes(...) gets inlined during compilation. is that ok? something we could certainly put in a readme, but it's something someone could easily miss when integrating auto instrumentation with their service.

@RonFed
Copy link
Contributor

RonFed commented Jan 30, 2024

I am thinking about attaching to the following functions:

  • func WithTimestamp(t time.Time) SpanEventOption, if this is not used we should use bpf_ktime_get_ns() for event timestamp.
  • func WithAttributes(attributes ...attribute.KeyValue) SpanStartEventOption (I think this one is also relevant to the Start function).

The tricky part is associating these calls with the span we are building in eBPF. One option that I can think of, is adding a hash map with keys as the goroutine pointer (similar to what get_consistent_key returns), and the value will be a pointer to the concrete option (the return pointer from the above functions). Then in the AdddEvent probe we can lookup these options, apply them and delete them from the map.

@RonFed nearly have something that works with having a separate probe for func WithAttributes(attributes ...attribute.KeyValue) SpanStartEventOption. i have an e2e test that works, but have a hard coded loop value that otherwise results in a max value being exceeded in a register for some reason. should hopefully figure that out soon. i also needed to build the go e2e test binary with inlining disabled because it turns out WithAttributes(...) gets inlined during compilation. is that ok? something we could certainly put in a readme, but it's something someone could easily miss when integrating auto instrumentation with their service.

It is better to find a function which doesn't get inlined if possible (I remember there is an another inner function which handles options, maybe worth looking into it).
In general, given an interface pointer finding out what the concrete type from eBPF could be very useful in many other contexts.

@asecondo
Copy link
Contributor

It is better to find a function which doesn't get inlined if possible (I remember there is an another inner function which handles options, maybe worth looking into it).
In general, given an interface pointer finding out what the concrete type from eBPF could be very useful in many other contexts.

the closest thing i could find to this is the underlying type WithAttributes(...) is using: func (o attributeOption) applyEvent(c EventConfig) EventConfig. however, this symbol is never making its way to the symbol table. there's an entry in the interface table (itab), but there's not an explicit symbol for attributeOption.applyEvent, even with inlining and optimizations. example output:

$ go build -gcflags="-l -N" -o main
$ go tool nm main | grep attributeOption
5a1a10 R go:itab.go.opentelemetry.io/otel/trace.attributeOption,go.opentelemetry.io/otel/trace.SpanStartEventOption

any ideas? i'm going to push on with an implementation that relies on inlines being disabled in the meantime just to get both of those 2 pieces working e2e. if we can come up with a more clever way to do this that doesn't involve inlines being disabled, that refactor should be invisible to the Go code and e2e tests. if it helps, i can push a draft MR so we can look at the code together.

@RonFed another unrelated question - any preferences on a max size for span events per span? i arbitrarily chose 4, but we can adjust the const as needed.

@RonFed
Copy link
Contributor

RonFed commented Jan 31, 2024

It is better to find a function which doesn't get inlined if possible (I remember there is an another inner function which handles options, maybe worth looking into it).

In general, given an interface pointer finding out what the concrete type from eBPF could be very useful in many other contexts.

the closest thing i could find to this is the underlying type WithAttributes(...) is using: func (o attributeOption) applyEvent(c EventConfig) EventConfig. however, this symbol is never making its way to the symbol table. there's an entry in the interface table (itab), but there's not an explicit symbol for attributeOption.applyEvent, even with inlining and optimizations. example output:

$ go build -gcflags="-l -N" -o main

$ go tool nm main | grep attributeOption

5a1a10 R go:itab.go.opentelemetry.io/otel/trace.attributeOption,go.opentelemetry.io/otel/trace.SpanStartEventOption

any ideas? i'm going to push on with an implementation that relies on inlines being disabled in the meantime just to get both of those 2 pieces working e2e. if we can come up with a more clever way to do this that doesn't involve inlines being disabled, that refactor should be invisible to the Go code and e2e tests. if it helps, i can push a draft MR so we can look at the code together.

@RonFed another unrelated question - any preferences on a max size for span events per span? i arbitrarily chose 4, but we can adjust the const as needed.

Sounds good

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 21, 2024

Closing in favor of #954

@MrAlias MrAlias closed this as not planned Won't fix, can't repro, duplicate, stale Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

3 participants