ElasticAPM receiver: handle transactions and spans #294

gregkalapos · 2025-01-13T19:06:10Z

This PR adds mapping for transactions and spans from the Elastic APM data model to OTel.

It also adds tests based on golden. The idea for testing: we reuse ndjson files from the apm-server repo and send those to the receiver which we compare by using golden.

TODO (will bring these up in meetings) - done:

Share attribute name constants with opentelemetry-lib - we do the reverse here what's the enricher does that - but we use the exact same Elastic specific attribute names. We could share all those constants across these 2 libs. PR: Create common package for reusing elastic specific attributes opentelemetry-lib#141 - The PR is merged, but opentelemetry-lib will need the next release. I have a todo for this in the PR, I'd merge as is and update later.
Testing infrustructure

Many things are still outside this PR, like errors, logs, metrics. I propose to handle those in follow-ups, as they are fairly independent.

receiver/elasticapmreceiver/internal/mappers/IntakeV2ToDerivedFields.go

receiver/elasticapmreceiver/internal/mappers/IntakeV2ToSemConv.go

receiver/elasticapmreceiver/internal/mappers/IntakeV2ToDerivedFields.go

receiver/elasticapmreceiver/receiver.go

receiver/elasticapmreceiver/internal/mappers/IntakeV2ToDerivedFields.go

dmathieu · 2025-02-14T08:39:34Z

receiver/elasticapmreceiver/receiver.go

 			s := ss.Spans().AppendEmpty()

+			traceId, err := traceIDFromHex(event.Trace.Id)
+			if err == nil {


I would keep the receiver code very small, and extract all the logic into internal methods, so they can be more easily tested. But maybe that's my preference.

Maybe something like the upstream translator package: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/jaeger/jaegerproto_to_traces.go#L27

An internal function that given an Elastic span/transaction event returns its corresponding ptrace.ResourceSpan

Yeah, I agree. I moved out all these lines into separate functions. One nit here: https://github.com/elastic/opentelemetry-collector-components/pull/294/files#r1963186038 - but I feel it's ok.

rogercoll · 2025-02-17T08:07:26Z

receiver/elasticapmreceiver/internal/mappers/intakeV2ToSemConv.go

+		attributes.PutStr(semconv.AttributeTelemetrySDKLanguage, event.Service.Language.Name)
+	}
+	attributes.PutStr(semconv.AttributeTelemetrySDKName, "ElasticAPM")
+	attributes.PutStr(semconv.AttributeDeploymentEnvironmentName, event.Service.Environment)


Is the event.Service.Environment always populated? Should we set the optional semconv.AttributeDeploymentEnvironmentName if empty?

You are right, thanks! I added a check here.

receiver/elasticapmreceiver/internal/mappers/intakeV2ToDerivedFields.go

rogercoll · 2025-02-17T08:37:55Z

receiver/elasticapmreceiver/internal/mappers/intakeV2ToSemConv.go

+func TranslateIntakeV2TransactionToOTelAttributes(event *modelpb.APMEvent, attributes pcommon.Map) {
+	if event.Http != nil && event.Http.Request != nil {
+		attributes.PutStr(semconv.AttributeHTTPRequestMethod, event.Http.Request.Method)
+		attributes.PutInt(semconv.AttributeHTTPResponseStatusCode, int64(event.Http.Response.StatusCode))


The function below TranslateIntakeV2SpanToOTelAttributes checks for event.Http.Response being non-nil, wdyt about unifying the HTTP attributes setter in a separate function?

Makes sense, moved it.

rogercoll · 2025-02-17T08:49:29Z

receiver/elasticapmreceiver/receiver.go

 			s := ss.Spans().AppendEmpty()

+			traceId, err := traceIDFromHex(event.Trace.Id)
+			if err == nil {


Maybe something like the upstream translator package: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/jaeger/jaegerproto_to_traces.go#L27

An internal function that given an Elastic span/transaction event returns its corresponding ptrace.ResourceSpan

rogercoll · 2025-02-17T08:55:32Z

receiver/elasticapmreceiver/receiver_test.go

+	nextTrace := new(consumertest.TracesSink)
+	rec, _ := factory.CreateTraces(context.Background(), set, cfg, nextTrace)
+
+	if err := rec.Start(context.Background(), componenttest.NewNopHost()); err != nil {


Wdyt of deferring the receiver shutdown just after starting it?

defer func() { if err := rec.Shutdown(context.Background()); err != nil { t.Errorf("Shutdown failed: %v", err) } }()

Love it! Updated.

receiver/elasticapmreceiver/receiver_test.go

rogercoll · 2025-02-17T09:05:29Z

receiver/elasticapmreceiver/receiver_test.go

+	receiver := extractElasticAPMReceiver(rec)
+	if receiver == nil {
+		t.Fatal("Failed to extract elasticAPMReceiver")
+	}
+
+	protocol := "https"
+	if receiver.cfg.TLSSetting == nil {
+		protocol = "http"
+	}
+	resp, err := http.Post(protocol+"://"+receiver.cfg.Endpoint+intakePath, "application/x-ndjson", bytes.NewBuffer(data))


What about explicitly setting the receiver server endpoint so we don't require this type assertion?
Something like:

testEndpoint := testutil.GetAvailableLocalAddress(t) rcvr, err := newElasticAPMReceiver(nil, &Config{ ServerConfig: confighttp.ServerConfig{ Endpoint: testEndpoint, }, }, receivertest.NewNopSettings())

then (always http)

resp, err := http.Post("http://"+testEndpoint+intakePath, "application/x-ndjson", bytes.NewBuffer(data))

by using the GetAvailableLocalAddress helper, we ensure there are no port collisions during the tests execution

Thanks, this is a good idea. I think this PR predates testutil.GetAvailableLocalAddress(t), so that's why my less nice solution here originally.

Updated the PR and now I use testutil.GetAvailableLocalAddress(t) - some things I adapted, since now http.Post(...) is in a different method (due to another point above), and I did not want to pass the endpoint string around, so I still rely on receiver.cfg.Endpoint - but the endpoint itself is now defined by testutil.GetAvailableLocalAddress(t).

I did not want to pass the endpoint string around, so I still rely on receiver.cfg.Endpoint - but the endpoint itself is now defined by testutil.GetAvailableLocalAddress(t).

But then you need to do a type assertion on the receiver interface, is there any reason why not to use the string endpoint?

My only thinking was to not pass too many parameters. On the other hand, passing the string is very lightweight compared to the type assertion.

Pushed an update to get rid of the type assertion and now we pass the string.

receiver/elasticapmreceiver/receiver.go

rogercoll

lgtm, added a couple of nits

rogercoll · 2025-02-20T16:54:48Z

receiver/elasticapmreceiver/receiver_test.go

+
+	actualTraces := nextTrace.AllTraces()[0]
+	expectedFile := filepath.Join(testData, expectedYamlFileName)
+	expectedTraces, _ := golden.ReadTraces(expectedFile)


I would handle this error just in case we produce an invalid expectedFile at some point

rogercoll · 2025-02-20T16:58:46Z

receiver/elasticapmreceiver/receiver_test.go

+	set := receivertest.NewNopSettings()
+	nextTrace := new(consumertest.TracesSink)
+	rec, _ := factory.CreateTraces(context.Background(), set, cfg, nextTrace)
+	receiver := rec.(*sharedcomponent.Component[*elasticAPMReceiver]).Unwrap()


I don't think this type assertion is needed, you could just use the testEndpoint variable instead of rec *elasticAPMReceiver in the runComparison function.

Ok, let's go with that #294 (comment)

rogercoll · 2025-02-20T17:01:51Z

receiver/elasticapmreceiver/receiver.go

 	r.settings.Logger.Info("Starting HTTP server", zap.String("endpoint", r.cfg.ServerConfig.Endpoint))
+
+	// TODO: Remove this once we have a proper way to configure TLS
+	r.cfg.TLSSetting = nil


nit: could we set this in the factory.go createDefaultConfig default method instead? (to avoid mutating the config after being parsed)

rogercoll · 2025-02-20T17:08:58Z

receiver/elasticapmreceiver/internal/mappers/intakeV2ToSemConv.go

+		setHttpAttributes(event, attributes)
+
+		if event.Url != nil {
+			attributes.PutStr(semconv.AttributeURLFull, event.Url.Full)


Not sure if any of this inner fields can be nil, if so we might need to add a guard function for all the setters.

This is actually a good point - we need to guard these. For now I just added another check. This was already setting empty strings.

In the spec the required fields are listed: https://github.com/elastic/apm-server/tree/main/docs/spec/v2

If any of those are missing, apm server rejects those documents. But url.full isn't required.

This whole question makes me think... we should probably discuss how to handle invalid documents - like when a required field is missing.

In any case - this specific one is handled.

Updated: opened https://github.com/elastic/opentelemetry-dev/issues/553?issue=elastic%7Copentelemetry-dev%7C752

gregkalapos self-assigned this Jan 13, 2025

AlexanderWert reviewed Jan 14, 2025

View reviewed changes

receiver/elasticapmreceiver/internal/mappers/IntakeV2ToDerivedFields.go Show resolved Hide resolved

gregkalapos mentioned this pull request Jan 14, 2025

Create common package for reusing elastic specific attributes elastic/opentelemetry-lib#141

Merged

gregkalapos force-pushed the elastic-apm-receiver branch from baeb9ac to 9e5c118 Compare February 13, 2025 17:23

gregkalapos marked this pull request as ready for review February 13, 2025 17:29

gregkalapos requested a review from a team as a code owner February 13, 2025 17:29

gregkalapos requested review from dmathieu, florianl and rogercoll February 13, 2025 17:29

rockdaboot reviewed Feb 14, 2025

View reviewed changes

receiver/elasticapmreceiver/receiver.go Outdated Show resolved Hide resolved

dmathieu reviewed Feb 14, 2025

View reviewed changes

simitt requested review from carsonip and lahsivjar February 17, 2025 07:25

rogercoll reviewed Feb 17, 2025

View reviewed changes

gregkalapos commented Feb 20, 2025

View reviewed changes

receiver/elasticapmreceiver/receiver.go Show resolved Hide resolved

gregkalapos added 14 commits February 20, 2025 16:11

ElasticAPM receiver: handle transactions and spans

ae86647

Update IntakeV2ToDerivedFields.go

b01eeeb

Move to SemConv v1.27.0

4febee7

Update IntakeV2ToDerivedFields.go

951be39

Add testing

ee037cc

Update receiver_test.go

c00ab6a

Handle more fields

c72f39d

Adapt comments

2aa77ce

fix tests

65ec5bb

Update receiver_test.go

357dccd

run porto

37a18bc

Log errors for id parsing

b772539

Follow snake case file name convention

e555d12

Review feedback

f193261

gregkalapos added 3 commits February 20, 2025 16:14

Update receiver_test.go

d3c85aa

Update receiver_test.go

cb7b2be

Use testutil.GetAvailableLocalAddress in tests

b0da9e1

gregkalapos force-pushed the elastic-apm-receiver branch from 2500b94 to b0da9e1 Compare February 20, 2025 15:57

Update receiver_test.go

893afeb

gregkalapos requested a review from rogercoll February 20, 2025 16:13

rogercoll approved these changes Feb 20, 2025

View reviewed changes

Review feedback

abad4fe

gregkalapos merged commit bfbc218 into elastic:main Feb 20, 2025
11 checks passed

gregkalapos deleted the elastic-apm-receiver branch February 20, 2025 18:14

gregkalapos mentioned this pull request Apr 22, 2025

Delete go.mod in root #537

Merged

ElasticAPM receiver: handle transactions and spans #294

ElasticAPM receiver: handle transactions and spans #294

Uh oh!

Conversation

gregkalapos commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rogercoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregkalapos Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gregkalapos commented Jan 13, 2025 •

edited

Loading

gregkalapos Feb 20, 2025 •

edited

Loading