-
Notifications
You must be signed in to change notification settings - Fork 81
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
The SDK should not import pdata
#1106
Comments
It is not possible to use a protobuf definition without included a protobuf library as a dependency. Therefore, it does not seem viable to use any protobuf representation is the long-term |
There are two possibilities I am currently looking into:
|
Comparison of
I'm not entirely sure that I am encoding things correctly with This also, doesn't represent the issue I had in trying to encode attributes (i.e. type AnyValue struct {
Value *isAnyValue_Value
} Or maybe there is also some equivalent to I don't plan to continue evaluation of |
It was also pointed out by @pellared that the |
No, it shouldn't be. The alternate approach being worked on supports those features just as well as pdata does. Those PRs are independent of this. This does block the |
Those PRs are using pdata-specific functions, right? Wouldn't that mean we will need to re-do them? |
Yes, they are using I don't think we need to block those PRs as they can be updated when the alternate is done. Otherwise, progress on the SDK integration testing and examples will be blocked without them. |
FWIW, the whole SDK (i.e. probe, start/end func, struct decl) will also need to be updated. |
Resolved by #1195 |
Currently, the SDK module imports
pdata
, which implicilty imports large modules (i.e.github.com/gogo/protobuf
,google.golang.org/grpc
,google.golang.org/protobuf
,gopkg.in/yaml.v3
):opentelemetry-go-instrumentation/sdk/go.mod
Lines 7 to 30 in d0db32b
This SDK is going to be imported into the global OTel Go API. Meaning it needs to be as light as possible. Having these imports is not going to be viable.
A new transport model needs to be explored. Possibly, this could relate to a replacement of the
Event
type as a project data-model.Part of #954
The text was updated successfully, but these errors were encountered: