Skip to content

Commit

Permalink
Adapt serializer and trace OTLP code to new changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@mx-psi
mx-psi committed Jun 20, 2022
1 parent 175c787 commit 99f0170
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 27 deletions.
20 changes: 13 additions & 7 deletions pkg/otlp/internal/serializerexporter/exporter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"context"
"fmt"

"github.com/DataDog/datadog-agent/pkg/otlp/model/source"
"github.com/DataDog/datadog-agent/pkg/otlp/model/translator"
"github.com/DataDog/datadog-agent/pkg/serializer"
"github.com/DataDog/datadog-agent/pkg/tagger/collectors"
Expand Down Expand Up @@ -51,14 +52,19 @@ func newDefaultConfig() config.Exporter {
}
}

var _ translator.HostnameProvider = (*hostnameProviderFunc)(nil)
var _ source.Provider = (*sourceProviderFunc)(nil)

// hostnameProviderFunc is an adapter to allow the use of a function as a translator.HostnameProvider.
type hostnameProviderFunc func(context.Context) (string, error)
// sourceProviderFunc is an adapter to allow the use of a function as a translator.HostnameProvider.
type sourceProviderFunc func(context.Context) (string, error)

// Hostname calls f.
func (f hostnameProviderFunc) Hostname(ctx context.Context) (string, error) {
return f(ctx)
// Source calls f and wraps in a source struct.
func (f sourceProviderFunc) Source(ctx context.Context) (source.Source, error) {
hostname, err := f(ctx)
if err != nil {
return source.Source{}, err
}

return source.Source{Kind: source.HostnameKind, Identifier: hostname}, nil
}

// exporter translate OTLP metrics into the Datadog format and sends
Expand All @@ -81,7 +87,7 @@ func translatorFromConfig(logger *zap.Logger, cfg *exporterConfig) (*translator.
}

options := []translator.Option{
translator.WithFallbackHostnameProvider(hostnameProviderFunc(util.GetHostname)),
translator.WithFallbackSourceProvider(sourceProviderFunc(util.GetHostname)),
translator.WithHistogramMode(histogramMode),
translator.WithDeltaTTL(cfg.Metrics.DeltaTTL),
}
Expand Down
41 changes: 21 additions & 20 deletions pkg/trace/api/otlp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"time"

"github.com/DataDog/datadog-agent/pkg/otlp/model/attributes"
"github.com/DataDog/datadog-agent/pkg/otlp/model/source"
"github.com/DataDog/datadog-agent/pkg/trace/api/apiutil"
"github.com/DataDog/datadog-agent/pkg/trace/config"
"github.com/DataDog/datadog-agent/pkg/trace/info"
Expand Down Expand Up @@ -207,17 +208,8 @@ func (o *OTLPReceiver) processRequest(protocol string, header http.Header, in pt
}
}

// OTLPIngestSummary returns a summary of the received resource spans.
type OTLPIngestSummary struct {
// Hostname indicates the hostname of the passed resource spans.
Hostname string
// Tags returns a set of Datadog-specific tags which are relevant for identifying
// the source of the passed resource spans.
Tags []string
}

// ReceiveResourceSpans processes the given rspans and sends them to writer.
func (o *OTLPReceiver) ReceiveResourceSpans(rspans ptrace.ResourceSpans, header http.Header, protocol string) OTLPIngestSummary {
func (o *OTLPReceiver) ReceiveResourceSpans(rspans ptrace.ResourceSpans, header http.Header, protocol string) source.Source {

This comment has been minimized.

Sign in to view

Copy link
@julien-lebot

julien-lebot Jun 21, 2022

Contributor

I find the API a bit weird that it processes an rspan and returns a source - is that the only thing we need to return ?

This comment has been minimized.

Sign in to view

Copy link
@gbbr

gbbr Jun 22, 2022 •
edited
Loading

Contributor

The location where this API is used (the datadogexporter) needs the resolved source (hostname, container, tags, etc) that results from the processing of these spans. Now, I know that "the caller needs that info" is not the best explanation for an API decision but we do need to return some sort of result from processing these spans. At the moment, we only need the source.

What if we changed the documentation to be:

// ReceiveResourceSpans processes the given rspans and returns the source that it identified from
// processing them.

We do need to explain the return value anyway in the doc.

Does that help at all @julien-lebot? Not sure what you were thinking. For now, this is the only thing we need to return. If we think there may be more things in the future, it may be better to keep OTLPIngestSummary with only one field: Source source.Source and allow adding more fields later on without breaking existing callers, if needed...

This comment has been minimized.

Sign in to view

Copy link
@julien-lebot

julien-lebot Jun 22, 2022

Contributor

Thanks for the explanation @gbbr, indeed I thought keeping OTLPIngestSummary (or OTLPIngestResult) was a bit more self-explanatory - at least for someone not very familiar with the code base.
But just changing the documentation would be fine too, so either way is fine with me 👍
// each rspans is coming from a different resource and should be considered
// a separate payload; typically there is only one item in this slice
attr := rspans.Resource().Attributes()
Expand All @@ -226,15 +218,15 @@ func (o *OTLPReceiver) ReceiveResourceSpans(rspans ptrace.ResourceSpans, header
rattr[k] = v.AsString()
return true
})
hostname, hostok := attributes.HostnameFromAttributes(attr, o.conf.OTLPReceiver.UsePreviewHostnameLogic)
src, srcok := attributes.SourceFromAttributes(attr, o.conf.OTLPReceiver.UsePreviewHostnameLogic)
hostFromMap := func(m map[string]string, key string) {
// hostFromMap sets the hostname to m[key] if it is set.
if v, ok := m[key]; ok {
hostname = v
hostok = true
src = source.Source{Kind: source.HostnameKind, Identifier: v}
srcok = true
}
}
if !hostok {
if !srcok {
hostFromMap(rattr, "_dd.hostname")
}
env := rattr[string(semconv.AttributeDeploymentEnvironment)]
Expand Down Expand Up @@ -272,7 +264,7 @@ func (o *OTLPReceiver) ReceiveResourceSpans(rspans ptrace.ResourceSpans, header
tracesByID[traceID] = pb.Trace{}
}
ddspan := o.convertSpan(rattr, lib, span)
if !hostok {
if !srcok {
// if we didn't find a hostname at the resource level
// try and see if the span has a hostname set
hostFromMap(ddspan.Meta, "_dd.hostname")
Expand Down Expand Up @@ -318,9 +310,21 @@ func (o *OTLPReceiver) ReceiveResourceSpans(rspans ptrace.ResourceSpans, header
if env == "" {
env = o.conf.DefaultEnv
}
if !hostok {

// Get the hostname or set to empty
// if source is empty
var hostname string
if srcok {
switch src.Kind {
case source.HostnameKind:
hostname = src.Identifier
default:
hostname = ""
}
} else {
hostname = o.conf.Hostname
}

p.TracerPayload = &pb.TracerPayload{
Hostname: hostname,
Chunks: traceChunks,
Expand All @@ -341,10 +345,7 @@ func (o *OTLPReceiver) ReceiveResourceSpans(rspans ptrace.ResourceSpans, header
default:
log.Warn("Payload in channel full. Dropped 1 payload.")
}
return OTLPIngestSummary{
Hostname: hostname,
Tags: attributes.RunningTagsFromAttributes(attr),
}
return src
}

// marshalEvents marshals events into JSON.
Expand Down

0 comments on commit 99f0170

Please sign in to comment.