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

Expose OpenTelemetryController as interface option to Instrumentation #1025

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Aug 19, 2024

Ref #1026

This does two things:

  • Create an interface for OpenTelemetryController so it can be overwritten (collector receiver needs to push to next consumer as ptrace, rather than exporting otlp to an endpoint)
  • Exposes the probe.Event struct (which is the parameter to the Trace() function in the OTelController interface)

@damemi damemi marked this pull request as ready for review August 21, 2024 12:53
@damemi damemi requested a review from a team August 21, 2024 12:53
Copy link
Contributor

@RonFed RonFed left a 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?)

@damemi
Copy link
Contributor Author

damemi commented Aug 28, 2024

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?)

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

import "go.opentelemetry.io/auto/internal/pkg/instrumentation/probe"

// Event represents a probed span event.
type Event = probe.Event
Copy link
Contributor

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.

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants