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

Replace probe.Event with ptrace.ScopeSpans #1207

Merged
merged 29 commits into from
Nov 4, 2024

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Oct 21, 2024

Resolve #1125

This implements part of #1177. It removes the probe.Event type and replaces its use with ptrace.ScopeSpans. It does not replace the Controller with a collector receiver and new Handler type (like #1177). The goal here is to lay the foundation from which we can build out the collector receiver and generalize the probe handler.

PR Size considerations

This PR is large. I have done my best to structure commits so they can be reviewed as independent additions. The commits are not self contained (most will not compile/test successfully). Because of this I know of no better way to split this PR into something smaller. Suggestions are welcome if reviews think it is needed and have them.

Important to note

For the most part, this PR is a direct translation from our Event type to ptrace. There are a few notable changes that have also been made.

Probe creations functions now accept a version

The probe processing logic now produces a scope defining a tracer. This means a version of the "SDK" is needed for that scope.

Probes that do not receive a tracer scope from their events will now be passed the auto version to add to their scope.

This restructure is something that will eventually be needed for #1105.

ptrace.ScopeSpans is used instead of ptrace.Traces

The ptrace.Traces data-type is not used as probes should not be defining resource level attributes. That is left to the Manager/Controller.

For things like the auto/sdk probe, it may be necessary to pass along the version of the sdk package used in the future. When needed, it can be added as a scope attribute (once scope attributes are supported in the Controller or its replacement).

Use net.SplitHostPort in grpc client probe

Instead of parsing the address with the strings package, use the dedicated net.SplitHostPort function to parse the remote address.

Check SpanKindServer in auto/sdk probe e2e test

The collector will default to SpanKindInternal when the span kind is not set. We were explicitly setting the main span to this kind before. Instead set it to another kind to validate functionality.

Fix expected flags in e2e tests

Many e2e tests had 768 (0b11000000) as their span flags. I'm not entirely sure why they were expecting this, nor how it was getting set. But, ultimately, the second least-significant digit is not an OTel recognized flag (yet). Therefore, it should not be set.

This data-model switch seems to have fixed this. The tests have been updated appropriately.

@MrAlias MrAlias force-pushed the pdata-event branch 9 times, most recently from ec6ad80 to f689b9a Compare October 25, 2024 16:47
@MrAlias MrAlias marked this pull request as ready for review October 25, 2024 17:07
@MrAlias MrAlias requested a review from a team as a code owner October 25, 2024 17:07
@MrAlias MrAlias force-pushed the pdata-event branch 3 times, most recently from 77e797b to 67f68b5 Compare October 28, 2024 17:04
@MrAlias MrAlias force-pushed the pdata-event branch 2 times, most recently from 689850e to 75ad546 Compare October 29, 2024 21:11
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! This looks good to me, it took a bit of time to review, I like the code design changes. I wasn't able to check every detail, but the tests pass and there are new unit tests for the new code.

go.mod Show resolved Hide resolved
internal/pkg/instrumentation/bpf/database/sql/probe.go Outdated Show resolved Hide resolved
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.

Added a few non-blocking comments. This looks awesome.

go.mod Show resolved Hide resolved
internal/pkg/instrumentation/probe/probe.go Outdated Show resolved Hide resolved
internal/pkg/instrumentation/probe/probe.go Show resolved Hide resolved
internal/pkg/opentelemetry/controller.go Show resolved Hide resolved
@MrAlias MrAlias merged commit 493c236 into open-telemetry:main Nov 4, 2024
26 checks passed
@MrAlias MrAlias deleted the pdata-event branch November 4, 2024 15:45
MrAlias added a commit to MrAlias/opentelemetry-go-instrumentation that referenced this pull request Nov 4, 2024
Resolve open-telemetry#1234

The `sdk/telemetry` package is no longer used in the `auto` package
since the merge of open-telemetry#1207. It is not needed outside of the `sdk` package
and needs to be removed prior to stabilization of the
`go.opentelemetry.io/auto/sdk` module.
MrAlias added a commit to MrAlias/opentelemetry-go-instrumentation that referenced this pull request Nov 4, 2024
Resolve open-telemetry#1234

The `sdk/telemetry` package is no longer used in the `auto` package
since the merge of open-telemetry#1207. It is not needed outside of the `sdk` package
and needs to be removed prior to stabilization of the
`go.opentelemetry.io/auto/sdk` module.
MrAlias added a commit to MrAlias/opentelemetry-go-instrumentation that referenced this pull request Nov 4, 2024
Resolve open-telemetry#1234

The `sdk/telemetry` package is no longer used in the `auto` package
since the merge of open-telemetry#1207. It is not needed outside of the `sdk` package
and needs to be removed prior to stabilization of the
`go.opentelemetry.io/auto/sdk` module.
MrAlias added a commit to MrAlias/opentelemetry-go-instrumentation that referenced this pull request Nov 4, 2024
Resolve open-telemetry#1234

The `sdk/telemetry` package is no longer used in the `auto` package
since the merge of open-telemetry#1207. It is not needed outside of the `sdk` package
and needs to be removed prior to stabilization of the
`go.opentelemetry.io/auto/sdk` module.
MrAlias added a commit to MrAlias/opentelemetry-go-instrumentation that referenced this pull request Nov 4, 2024
Resolve open-telemetry#1234

The `sdk/telemetry` package is no longer used in the `auto` package
since the merge of open-telemetry#1207. It is not needed outside of the `sdk` package
and needs to be removed prior to stabilization of the
`go.opentelemetry.io/auto/sdk` module.
@MrAlias MrAlias added this to the v0.17.0-alpha milestone Nov 4, 2024
MrAlias added a commit that referenced this pull request Nov 5, 2024
Resolve #1234

The `sdk/telemetry` package is no longer used in the `auto` package
since the merge of #1207. It is not needed outside of the `sdk` package
and needs to be removed prior to stabilization of the
`go.opentelemetry.io/auto/sdk` module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[Spike] Alternate data model
3 participants