add otel tracing support #182

endorama · 2024-06-12T07:28:36Z

Add otel tracing support as a secondary tracing choice.

By default the current behaviour is retained: tracing with Elastic APM is preferred over otel tracing.
If Elastic APM tracing is not configured but otel tracing is, leverage otel tracing.

If no tracing is configured keep the current behaviour of skipping tracing.

Closes #45

If APM tracer is not set and an otel tracer is configured use it to trace the Appender.flush execution

As we want to support otel tracing report tracing enabled if its configured. otel tracer will be checked only if there is no APM tracer configured.

endorama · 2024-06-12T07:34:13Z

appender.go

+		logger = logger.With(
+			zap.String("traceId", span.SpanContext().TraceID().String()),
+			zap.String("spanId", span.SpanContext().SpanID().String()),
+		)


For reviewers: A otel zap bridge exists at https://github.com/uptrace/opentelemetry-go-extra/tree/main/otelzap
This implementation is not compatible with a standard zap logger though, as it wraps the logger in a custom otelzap.Logger type. Passing the context also requires to use an additional .Ctx(ctx) call that is not supported by zap.Logger.
Due to this I chose not to use it an instead provide correlation information as standard otel attributes. I expect this to retain the same behaviour that we get with logger.With(apmzap.TraceContext(ctx)...) on line 320.

appender_test.go

config.go

appender.go

As we are using a span previously initialized we can ensure it's active before trying to record events on it.

As it's an in memory exporter shutting it down means we can't get spans from it.

Instead of accepting a Tracer use more idiomatic code by accepting a TracerProvider from which a Tracer is initialized in New()

span.RecordError does not set span status, which must be called manually.

endorama · 2024-06-13T10:51:42Z

appender.go

@@ -316,6 +326,19 @@ func (a *Appender) flush(ctx context.Context, bulkIndexer *BulkIndexer) error {
 		// Add trace IDs to logger, to associate any per-item errors
 		// below with the trace.
 		logger = logger.With(apmzap.TraceContext(ctx)...)
+	} else if a.otelTracingEnabled() {
+		// NOTE: this is missing transaction type information. How is this conveyed in otel?
+		ctx, span = a.tracer.Start(ctx, "docappender.flush", trace.WithAttributes(


For reviewers: compared to APM this is missing "span type" information, but to my understanding this is delegated to an eventual otel<>Elastic APM bridge so is not a concern here.

axw · 2024-06-17T01:06:30Z

config.go

+	// OtelTracerProvider holds an optional otel TracerProvider for tracing
+	// flush requests.
+	//
+	// If OtelTracerProvider is nil, requests will not be traced.
+	// To use this provider Tracer must be nil.
+	OtelTracerProvider trace.TracerProvider


Maybe call this TracerProvider, with the expectation that we'll remove Tracer in the future.

done in 6cc6cea (#182)

axw · 2024-06-17T01:06:53Z

config.go

+	// If Tracer is nil, requests will not be traced. Note however that
+	// OtelTracerProvider may not be nil, in which case the request will
+	// be traced by a different tracer.


I'm tempted to add a deprecation notice to this, WDYT?

I agree, done in a5fa1ad (#182)

carsonip · 2024-06-17T13:10:07Z

appender.go

@@ -85,6 +87,8 @@ type Appender struct {
 	metrics               metrics
 	mu                    sync.Mutex
 	closed                chan struct{}
+
+	tracer trace.Tracer


nit: a.config.tracer is an Elastic APM tracer but a.tracer is an otel tracer, might be confusing to one. Is it possible to make it clearer?

ah, I see that it was discussed. So maybe add a comment that this should not be confused with a.config.Tracer?

Thanks for the suggestion, I approved it. Once the migration to otel is completed I think this will look way less confusing, but meanwhile a clarification comment helps for sure.

appender.go

Co-authored-by: Carson Ip <carsonip@users.noreply.github.com>

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [github.com/elastic/go-docappender/v2](https://github.com/elastic/go-docappender) | `v2.1.4` -> `v2.2.0` | [![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2felastic%2fgo-docappender%2fv2/v2.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2felastic%2fgo-docappender%2fv2/v2.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2felastic%2fgo-docappender%2fv2/v2.1.4/v2.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2felastic%2fgo-docappender%2fv2/v2.1.4/v2.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>elastic/go-docappender (github.com/elastic/go-docappender/v2)</summary> ### [`v2.2.0`](https://github.com/elastic/go-docappender/releases/tag/v2.2.0) [Compare Source](https://github.com/elastic/go-docappender/compare/v2.1.4...v2.2.0) ##### What's Changed - fix: update CODEOWNERS by [@kruskall](https://github.com/kruskall) in [elastic/go-docappender#181 - bulk_indexer: track all response status codes by [@marclop](https://github.com/marclop) in [elastic/go-docappender#177 - build(deps): bump github.com/klauspost/compress from 1.17.8 to 1.17.9 by [@dependabot](https://github.com/dependabot) in [elastic/go-docappender#183 - add otel tracing support by [@endorama](https://github.com/endorama) in [elastic/go-docappender#182 ##### New Contributors - [@endorama](https://github.com/endorama) made their first contribution in [elastic/go-docappender#182 **Full Changelog**: elastic/go-docappender@v2.1.4...v2.2.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com> Co-authored-by: Yang Song <songy23@users.noreply.github.com>

…lemetry#33967) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [github.com/elastic/go-docappender/v2](https://github.com/elastic/go-docappender) | `v2.1.4` -> `v2.2.0` | [![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2felastic%2fgo-docappender%2fv2/v2.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2felastic%2fgo-docappender%2fv2/v2.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2felastic%2fgo-docappender%2fv2/v2.1.4/v2.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2felastic%2fgo-docappender%2fv2/v2.1.4/v2.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>elastic/go-docappender (github.com/elastic/go-docappender/v2)</summary> ### [`v2.2.0`](https://github.com/elastic/go-docappender/releases/tag/v2.2.0) [Compare Source](https://github.com/elastic/go-docappender/compare/v2.1.4...v2.2.0) ##### What's Changed - fix: update CODEOWNERS by [@&open-telemetry#8203;kruskall](https://github.com/kruskall) in [elastic/go-docappender#181 - bulk_indexer: track all response status codes by [@&open-telemetry#8203;marclop](https://github.com/marclop) in [elastic/go-docappender#177 - build(deps): bump github.com/klauspost/compress from 1.17.8 to 1.17.9 by [@&open-telemetry#8203;dependabot](https://github.com/dependabot) in [elastic/go-docappender#183 - add otel tracing support by [@&open-telemetry#8203;endorama](https://github.com/endorama) in [elastic/go-docappender#182 ##### New Contributors - [@&open-telemetry#8203;endorama](https://github.com/endorama) made their first contribution in [elastic/go-docappender#182 **Full Changelog**: elastic/go-docappender@v2.1.4...v2.2.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com> Co-authored-by: Yang Song <songy23@users.noreply.github.com>

endorama added 4 commits June 11, 2024 17:36

add OtelTracer config

9dabb54

update Appender.flush to use otel tracer

dabb278

If APM tracer is not set and an otel tracer is configured use it to trace the Appender.flush execution

detect otel tracer in Appender.tracingEnabled

cab7923

As we want to support otel tracing report tracing enabled if its configured. otel tracer will be checked only if there is no APM tracer configured.

use otel tracer

c97556c

obltmachine added the safe-to-test Automated label for running bench-diff on forked PRs label Jun 12, 2024

endorama added 2 commits June 12, 2024 09:30

otelTracingEnabled

49fc649

add test

046ad35

endorama force-pushed the otel-tracing branch from cd38cce to 046ad35 Compare June 12, 2024 07:30

endorama commented Jun 12, 2024

View reviewed changes

appender_test.go Outdated Show resolved Hide resolved

endorama commented Jun 12, 2024

View reviewed changes

config.go Outdated Show resolved Hide resolved

endorama commented Jun 12, 2024

View reviewed changes

appender.go Outdated Show resolved Hide resolved

endorama added 13 commits June 12, 2024 09:40

check if span is recording

87d8cb3

As we are using a span previously initialized we can ensure it's active before trying to record events on it.

use span variable

827e13e

do not shutdown exporter

957d4cd

As it's an in memory exporter shutting it down means we can't get spans from it.

add tests

c0064a3

pass tracer provider to Config

9fa8259

Instead of accepting a Tracer use more idiomatic code by accepting a TracerProvider from which a Tracer is initialized in New()

test span status

97baa63

document OtelTracerProvider

97e7d5f

move test code to file end

735aebf

test failure case

bbee58b

setStatus must be called manually

11de058

span.RecordError does not set span status, which must be called manually.

document otelTracingEnabled

5867c7b

update go.mod

a9e18c2

remove comment

27b0b86

endorama commented Jun 13, 2024

View reviewed changes

endorama marked this pull request as ready for review June 13, 2024 10:52

endorama requested a review from a team as a code owner June 13, 2024 10:52

Merge branch 'main' into otel-tracing

3038196

endorama mentioned this pull request Jun 13, 2024

Add support for OTel tracing #45

Closed

kruskall previously approved these changes Jun 13, 2024

View reviewed changes

1pkg previously approved these changes Jun 13, 2024

View reviewed changes

axw previously approved these changes Jun 17, 2024

View reviewed changes

endorama added 2 commits June 17, 2024 09:21

OtelTracerProvider -> TracerProvider

6cc6cea

add deprecation notice to Tracer

a5fa1ad

endorama dismissed stale reviews from axw, 1pkg, and kruskall via a5fa1ad June 17, 2024 07:21

kruskall previously approved these changes Jun 17, 2024

View reviewed changes

carsonip reviewed Jun 17, 2024

View reviewed changes

appender.go Show resolved Hide resolved

Update appender.go

deb950b

Co-authored-by: Carson Ip <carsonip@users.noreply.github.com>

endorama dismissed kruskall’s stale review via deb950b June 17, 2024 13:21

kruskall previously approved these changes Jun 17, 2024

View reviewed changes

go fmt

a1aa8e8

endorama dismissed kruskall’s stale review via a1aa8e8 June 17, 2024 14:06

carsonip approved these changes Jun 17, 2024

View reviewed changes

endorama merged commit 0c05046 into elastic:main Jun 17, 2024
5 checks passed

endorama deleted the otel-tracing branch June 17, 2024 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add otel tracing support #182

add otel tracing support #182

endorama commented Jun 12, 2024 •

edited

Loading

endorama Jun 12, 2024 •

edited

Loading

endorama Jun 13, 2024

axw Jun 17, 2024

endorama Jun 17, 2024

axw Jun 17, 2024

endorama Jun 17, 2024

carsonip Jun 17, 2024 •

edited

Loading

carsonip Jun 17, 2024

endorama Jun 17, 2024

add otel tracing support #182

add otel tracing support #182

Conversation

endorama commented Jun 12, 2024 • edited Loading

endorama Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

endorama Jun 13, 2024

Choose a reason for hiding this comment

axw Jun 17, 2024

Choose a reason for hiding this comment

endorama Jun 17, 2024

Choose a reason for hiding this comment

axw Jun 17, 2024

Choose a reason for hiding this comment

endorama Jun 17, 2024

Choose a reason for hiding this comment

carsonip Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

carsonip Jun 17, 2024

Choose a reason for hiding this comment

endorama Jun 17, 2024

Choose a reason for hiding this comment

endorama commented Jun 12, 2024 •

edited

Loading

endorama Jun 12, 2024 •

edited

Loading

carsonip Jun 17, 2024 •

edited

Loading