feat(otlp): experimental otlp support #2177
Conversation
petethepig
commented
Jul 27, 2023
•
petethepig
commented
- very much a draft at this point.
- requires a copy of https://github.com/open-telemetry/opentelemetry-proto-profile to be adjacent to the directory
- works with feat(otlp): experimental otlp support pyroscope-go#36 and Adds profiles as another signal petethepig/opentelemetry-collector#1
pkg/api/api.go
Outdated
| a.RegisterRoute("/ingest", pyroscopeHandler, true, true, "POST") | ||
| a.RegisterRoute("/pyroscope/ingest", pyroscopeHandler, true, true, "POST") | ||
| pushv1connect.RegisterPusherServiceHandler(a.server.HTTP, d, a.grpcAuthMiddleware) | ||
| a.RegisterRoute("/distributor/ring", d, false, true, "GET", "POST") | ||
| a.indexPage.AddLinks(defaultWeight, "Distributor", []IndexPageLink{ | ||
| {Desc: "Ring status", Path: "/distributor/ring"}, | ||
| }) | ||
|
|
||
| pprofileotlp.RegisterGRPCServer(a.server.GRPC, otlpHandler) |
There was a problem hiding this comment.
Just so you know I don't think we expose GRPC endpoint anywhere in our infra nor in helm.
There was a problem hiding this comment.
@cyriltovena yeah we might need a separate grpc server
go.mod
Outdated
| go.opentelemetry.io/collector/pdata v1.0.0-rcv0012 | ||
| ) | ||
|
|
||
| replace go.opentelemetry.io/collector/pdata => ../opentelemetry-collector/pdata |
There was a problem hiding this comment.
to be clear, we won't have this dependency once this is ready to merge
pkg/ingester/otlp/ingest_handler.go
Outdated
| // TODO(@petethepig): make it tenant-aware | ||
| ctx = tenant.InjectTenantID(ctx, tenant.DefaultTenantID) | ||
|
|
||
| h.log.Log("msg", "Export called") |
There was a problem hiding this comment.
we should remove this and any other logging messages like this
pkg/validation/validate.go
Outdated
| return NewErrorf(MalformedProfile, "function id is 0") | ||
| } | ||
| } | ||
| // for _, location := range prof.Location { |
There was a problem hiding this comment.
I commented it out because something was breaking... that's not good, ideally we should figure out how to handle this properly
There was a problem hiding this comment.
Hmm.. I uncommented it, but (as it is now) nothing fails 🤔
pkg/ingester/otlp/ingest_handler.go
Outdated
| }) | ||
| labelsDst = append(labelsDst, &typesv1.LabelPair{ | ||
| Name: "service_name", | ||
| Value: "otlp_test_app4", |
There was a problem hiding this comment.
@marcsanmi This and other labels is something we still need to figure out. There's multiple ways to add attributes in OTLP. Here's an example OTLP file encoded with YAML: https://gist.github.com/petethepig/0b5a0d71f8f5350d2082169690154f71
Look for attributes, attributetable and attributes.
Some labels are required in pyroscope. I would go with the defaults for __name__, service_name, __delta__ and pyroscope_spy listed here, except for service_name. it should be something else, maybe unknown?
There was a problem hiding this comment.
Done in bfa3f6c. I kept the defaults, but also added the capability to get service name from resource attributes, otherwise defaulting to unknown (even adding attributes processor is not supported in the collector profiles pipeline AFAIK)
pkg/ingester/otlp/ingest_handler.go
Outdated
| func (h *ingestHandler) Export(ctx context.Context, er *pprofileotlp.ExportProfilesServiceRequest) (*pprofileotlp.ExportProfilesServiceResponse, error) { | ||
|
|
||
| // TODO(@petethepig): make it tenant-aware | ||
| ctx = tenant.InjectTenantID(ctx, tenant.DefaultTenantID) |
There was a problem hiding this comment.
We should look into this and compare it to how it's done in other handlers too before merging
| processedKeys := make(map[string]bool) | ||
|
|
||
| // Add resource attributes | ||
| labels = appendAttributesUnique(labels, rp.Resource.GetAttributes(), processedKeys) |
There was a problem hiding this comment.
Right now we're adding all attributes as labels... do we want to set some limit?
pkg/ingester/otlp/ingest_handler.go
Outdated
| _, ctx, err := user.ExtractFromGRPCRequest(ctx) | ||
| if err != nil { | ||
| level.Error(h.log).Log("msg", "failed to extract tenant ID from GRPC request", "err", err) | ||
| return &pprofileotlp.ExportProfilesServiceResponse{}, fmt.Errorf("failed to extract tenant ID from GRPC request: %w", err) |
There was a problem hiding this comment.
Do we want to fail when we aren't able to extract tenant id? Or adding the default fallback?
