-
Notifications
You must be signed in to change notification settings - Fork 83
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
Expose OpenTelemetryController as interface option to Instrumentation #1025
base: main
Are you sure you want to change the base?
Conversation
5691e3f
to
d286af5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As we talked in the SIG, I think we need to add some comments about the probe.Event
struct and its fields, and maybe rename them since SpanEvent
is a misleading name (maybe SpanData
?)
d286af5
to
b8e9fff
Compare
I added some comments, but don't want to make too many changes like renaming the struct in this PR if that's ok with you |
pkg/probe/probe.go
Outdated
import "go.opentelemetry.io/auto/internal/pkg/instrumentation/probe" | ||
|
||
// Event represents a probed span event. | ||
type Event = probe.Event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant to export this type.
I just noticed that the start/end time are not actually unix timestamps, but the relative offset from startup.
I think we need to better evaluate the structure of this event type before we publish something others will rely on. It might make the most sense if we actually use a proto representation of spans similar to the collector's pdata
approach. That way we could avoid deserialization if an exporter speaks that protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MrAlias I pushed an update adding a wrapper struct that gives us a public abstraction from the internal types. Ideally I think we should eventually just take a good look at the internal types and make them public, but something like this could give us a nice bridge to that. Wdyt? Open to ideas
b8e9fff
to
8a29214
Compare
Ref #1026
This does two things:
probe.Event
struct (which is the parameter to theTrace()
function in the OTelController interface)