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

The SDK should not import pdata #1106

Closed
MrAlias opened this issue Sep 16, 2024 · 10 comments
Closed

The SDK should not import pdata #1106

MrAlias opened this issue Sep 16, 2024 · 10 comments
Assignees

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Sep 16, 2024

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

require (
github.com/stretchr/testify v1.9.0
go.opentelemetry.io/collector/pdata v1.15.0
go.opentelemetry.io/otel v1.30.0
go.opentelemetry.io/otel/trace v1.30.0
)
require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/kr/text v0.2.0 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
golang.org/x/net v0.26.0 // indirect
golang.org/x/sys v0.21.0 // indirect
golang.org/x/text v0.16.0 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240604185151-ef581f913117 // indirect
google.golang.org/grpc v1.66.0 // indirect
google.golang.org/protobuf v1.34.2 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)

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

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 16, 2024

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

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 16, 2024

There are two possibilities I am currently looking into:

  • Using encoding/gob
  • Using encoding/json with an OTLP encoding

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 18, 2024

Comparison of encoding/gob vs encoding/json for serialization of SDK data-model:

$ go test -run="^$" -bench=.
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/auto/internal/pkg/data
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkGOBMarshalUnmarshal-8    	    4609	    266378 ns/op	   35710 B/op	     778 allocs/op
BenchmarkJSONMarshalUnmarshal-8   	   41809	     26202 ns/op	    1793 B/op	      20 allocs/op
PASS

I'm not entirely sure that I am encoding things correctly with gob, but it appears to be significantly less performant.

This also, doesn't represent the issue I had in trying to encode attributes (i.e. AnyValue). I think the data-model would need to be updated so the Value field has a pointer to the interface:

type AnyValue struct {
	Value *isAnyValue_Value
}

Or maybe there is also some equivalent to UnmarshalJSON.

I don't plan to continue evaluation of gob, though, given the performance difference.

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 19, 2024

It was also pointed out by @pellared that the encoding/gob use would have compatibility issues. If the sender/receiver used different versions of the data-model they would not be compatible.

@RonFed
Copy link
Contributor

RonFed commented Sep 21, 2024

Is this blocking PRs like #1110, #1111 and #1112?

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 21, 2024

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 sdk package from being released though given the global API cannot depend on it until this dependency is removed.

@RonFed
Copy link
Contributor

RonFed commented Sep 21, 2024

Those PRs are using pdata-specific functions, right? Wouldn't that mean we will need to re-do them?

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 21, 2024

Yes, they are using pdata functions, and they will need to be updated when alternate data-model is added. However, the overall functionality and testing should remain similar, if not the same, with a replacement of the data-model.

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.

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 21, 2024

FWIW, the whole SDK (i.e. probe, start/end func, struct decl) will also need to be updated.

@MrAlias MrAlias mentioned this issue Oct 3, 2024
2 tasks
@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 22, 2024

Resolved by #1195

@MrAlias MrAlias closed this as completed Oct 22, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Auto Instrumentation: Beta Oct 22, 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

2 participants