Introduce otlp/http support in OTelTraceSource by TomasLongo · Pull Request #5322 · opensearch-project/data-prepper

TomasLongo · 2025-01-10T09:00:19Z

Description

Introduce http endpoint to support otlp/http for the otel trace source. So far, this draft PR contains:

A single armeria server listening on a port
A HTTP Service
- listening under /opentelemetry.proto.collector.trace.v1.TraceService/Export
- processing ExportTraceServiceRequest
- supporting basic auth/tls
(not yet complete)Testing of the http service
updated otel-proto specs

This draft PR serves as starting for further discussions

Configuration of HTTP and gRPC Service

Currently, the source config is responsible for setting up a working gRPC service. Now, an HTTP Service has to be configured as well.

As far as this PR is concerned, we can assign the current config items into two groups:

Config items for gRPC specific features (e.g. unframed_requests, proto_reflection_service)
Config items relevant for both services (e.g. compression, authentication) and should/could

This leads to the questions how the structure of the config should look like in the future

Create distinct sections for every endpoint

port: 123  
thread_count: 123  
...  
http:  
  path: /path  compression: gzip  authentication:    http_basic:grpc:  
  compression: gzip  proto_reflection_service: true  authentication:    http_basic:

Let the endpoint share as much of the current config as possible

e.g. features like compression, authentication

Do we want to keep unframed requests

My understanding is that unframed requests enable clients to send plain http requests to the gRPC endpoint and would be rendered deprecated by this PR. However, there might be more to this feature which I'm currently unaware of.

Usage of the plugin mechanism to handle configs

The Method HttpService.createAuthenticationProvider contains a comment on how the handling of the basic auth config could be made less complex.

The basic idea is to not treat simple configs as plugins. The overall developer experience could benefit from using the simple parsing mechanism that is already used for other configs (like RetryInfo).

Again, there could be reasons for using the plugin mechanism which I'm not aware of.

Issues Resolved

Resolves #4983

Related to #5259

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

KarstenSchnitter

Thanks for providing this PR. This is intended to start a discussion about this change. @dlvenable we would like your opinion as well as of all other maintainers. There are still a couple of TODOs in the comment, that need to be addressed. I think, they mark points waiting for clarification.

Tomas, can you explain a little bit more about the upgrade of the otlp-proto classes. Did you need to remove the instrumentation library support because of that? Do you know, how this change behaves if the old format is ingested?

.../integrationTest/java/org/opensearch/dataprepper/plugins/kafka/buffer/KafkaBufferOTelIT.java

...st/java/org/opensearch/dataprepper/plugins/processor/otelmetrics/MetricsPluginGaugeTest.java

...c/test/java/org/opensearch/dataprepper/plugins/source/otelmetrics/OTelMetricsSourceTest.java

...a/org/opensearch/dataprepper/plugins/source/otelmetrics/OTelMetricsSource_RetryInfoTest.java

...proto-common/src/main/java/org/opensearch/dataprepper/plugins/otel/codec/OTelProtoCodec.java

TomasLongo · 2025-01-17T08:57:08Z

Tomas, can you explain a little bit more about the upgrade of the otlp-proto classes. Did you need to remove the instrumentation library support because of that?

Yes: According to the deprecation message in 0.16.0 InstrumentationLibrary was removed in June 15, 2022 (pretty specific). The now used version (1.3.2-alpha) does indeed not carry it anymore.

Do you know, how this change behaves if the old format is ingested?

My Tests so far:

message carrying scopeSpans along with instrumentationLibrarySpans
-- instrumentationLibrarySpans are simply ignored. The sink will receive the data from the scope spans
message carrying only instrumentationLibrarySpans
-- Sink will receive nothing, since the current implementation looks for scopeSpans inside the resourceSpans and returns an empty list if it can find none

All the tests have been made against the new http endpoint (using json as input). I have yet to test behaviour of the gRPC endpoint (I don't expect a different behaviour, though, since both endpoints use the same decoding functionality)

[UPDATE ON TESTING 2025/01/17]
Testing the gRPC endpoint brought no surprises. Clients with outdated protobuf specifications will still be able to use data-prepper as long as their messages come packed with ScopeSpans

The spec of 0.16.0 carried details on how senders and receivers should behave during the grace period:

During the grace period the following rules SHOULD be followed:

For Binary Protobufs

Binary Protobuf senders SHOULD NOT set instrumentation_library_spans. Instead
scope_spans SHOULD be set.

Binary Protobuf receivers SHOULD check if instrumentation_library_spans is set
and scope_spans is not set then the value in instrumentation_library_spans
SHOULD be used instead by converting InstrumentationLibrarySpans into ScopeSpans.
If scope_spans is set then instrumentation_library_spans SHOULD be ignored.

For JSON

JSON senders that set instrumentation_library_spans field MAY also set
scope_spans to carry the same spans, essentially double-publishing the same data.
Such double-publishing MAY be controlled by a user-settable option.
If double-publishing is not used then the senders SHOULD set scope_spans and
SHOULD NOT set instrumentation_library_spans.

JSON receivers SHOULD check if instrumentation_library_spans is set and
scope_spans is not set then the value in instrumentation_library_spans
SHOULD be used instead by converting InstrumentationLibrarySpans into ScopeSpans.
If scope_spans is set then instrumentation_library_spans field SHOULD be ignored.

According to that comment, the data-prepper behaves correctly now, and did adhere to the specfication in the past (by using either ScopeSpans or InstrumentationLibrarySpans).

IMO, if Data-Prepper clients followed the recommendations, there should be no friction upgrading the proto classes.

TomasLongo · 2025-01-17T09:28:19Z

I discovered a comment that could be important for backward compatibility in the 0.16.0 deprecation notice:

InstrumentationLibrarySpans is wire-compatible with ScopeSpans for binary Protobuf format

I'm not that fluent in protbuf speech yet to really grasp the implication of that statement. Does wire-compatible mean, that the binary representation of InstrumentationLibrarySpans and ScopeSpans does look the same and that receivers can therefore simply decode incoming InstrumentationLibrarySpans into ScopeSpans in their code without any further effort?

KarstenSchnitter · 2025-01-21T12:58:14Z

@TomasLongo I will check the wire compatibility. Can you have a look at the OTelTraceSource_*Test test cases. For me the first test-case in ./gradlew ":data-prepper-plugins:otel-trace-source:test" fails causing all subsequent tests to fail as well:

./gradlew ":data-prepper-plugins:otel-trace-source:test"

> Task :data-prepper-plugins:otel-trace-source:test

OTelTraceSourceTest > testOptionalHttpAuthServiceInPlaceWithUnauthenticatedDisabled() FAILED
    java.lang.NullPointerException: Cannot invoke "Object.toString()" because the return value of "java.util.Map.get(Object)" is null

OTelTraceSourceTest > testHealthCheckUnauthAllowed() FAILED
    java.lang.AssertionError: Http Status
    Expected: <200 OK>
         but: was <401 Unauthorized>

OTelTraceSourceTest > testServerConnectionsMetric() FAILED
    java.lang.RuntimeException: java.util.concurrent.ExecutionException: io.netty.channel.unix.Errors$NativeIoException: bind(..) failed: Address already in use

        Caused by:
        java.util.concurrent.ExecutionException: io.netty.channel.unix.Errors$NativeIoException: bind(..) failed: Address already in use

            Caused by:
            io.netty.channel.unix.Errors$NativeIoException: bind(..) failed: Address already in use

OTelTraceSourceTest > testOptionalHttpAuthServiceInPlace() FAILED
    java.lang.NullPointerException: Cannot invoke "Object.toString()" because the return value of "java.util.Map.get(Object)" is null

OTelTraceSourceTest > testHttpFullJsonWithCustomPathAndAuthHeader_with_successful_response() FAILED
    java.lang.RuntimeException: java.util.concurrent.ExecutionException: io.netty.channel.unix.Errors$NativeIoException: bind(..) failed: Address already in use

        Caused by:
        java.util.concurrent.ExecutionException: io.netty.channel.unix.Errors$NativeIoException: bind(..) failed: Address already in use

            Caused by:
            io.netty.channel.unix.Errors$NativeIoException: bind(..) failed: Address already in use

[...]

This is also causing the failing tests on the build system.

KarstenSchnitter · 2025-01-21T15:31:09Z

...urce/src/main/java/org/opensearch/dataprepper/plugins/source/oteltrace/http/HttpService.java

+        //   - it becomes testable
+        // cons:
+        //   - currently tied to one impl by using 'new'.
+        return new HttpBasicArmeriaHttpAuthenticationProvider(new HttpBasicAuthenticationConfig(pluginSettings.get("username").toString(), pluginSettings.get("password").toString()));


Check this line. This causes an NPE in testing, when pluginsSettings is an empty map. Run test case OTelTraceSourceTest.testOptionalHttpAuthServiceInPlaceWithUnauthenticatedDisabled.

This is one of the two tests (the other beeing testOptionalHttpAuthServiOceInPlace) that causes NPEs. I disabled them for now! However, we should discuss if they are still needed.

The reason is, that the test assumes an invalid auth-config, which data-prepper refuses to ingest in production. The config in the test looks as follows:

otel_trace_source:
authentication:
test:
The tests are nonetheless green in main, because the critical code, the PluginFactory, is completely mocked away in the test. This branch produces NPEs, because the new HTTPService does not use the Pluginfactory to instantiate the AuthProvider.

IMO, this tests could be removed.

@dlvenable what is your take on that?

@TomasLongo , We use plugins for authentication to allow for custom authentication which may not always be available in the open source project. We should continue that pattern here as well. Also, we should avoid having passwords and usernames stored in the code.

ok. done. The auth provider us now created via the plugin factory.

I'm not sure what you mean by "passwords and usernames stored in code", though. I could not find any credentials stored in the code.

kkondaka · 2025-02-03T20:07:53Z

@TomasLongo Some tests (like ':e2e-test:trace:compileIntegrationTestJava') are failing. Please check

kkondaka · 2025-02-07T21:10:04Z

@TomasLongo @KarstenSchnitter @dlvenable should we split this in to 2 PRs? One for just updated OTEL spec version? And another for OTLP_HTTP support?

TomasLongo · 2025-02-13T08:17:59Z

@kkondaka @KarstenSchnitter @dlvenable

As suggested, I've created a new PR #5434, containing only the updated otel proto buf spec. I've rebased this branch in order to simply create a patch of the changes for the new spec. You'll need to refetch the branch if you have checked it out before.

I'll cleanup this PR here and incorporate your suggestions.

However, we still have to address the configuration of the new http service. Any thoughts on that topic? Or should I come up with with a first draft for this?

TomasLongo · 2025-02-14T12:10:16Z

I've cleaned up the PR as far as I could, without needing further input from maintainer side. The remaining todos cover the following topics:

how to design the otel-trace-source-config now that the http service entered the game
do we still need unframed requests?

TomasLongo · 2025-02-21T13:30:17Z

Here's a first draft, of how the config might look like. The assumption is, that both endpoints serve the same purpose, so they should share as much config as possible. That's why things like compression, health_check_service, etc are shared.

source:
    otel_trace_source:
        retry_info:
        compression: 'none' | 'gzip'
        ssl: boolean
        request_timeout: int
        authentication:
        health_check_service: boolean
        http:
          path: string
        grpc:
          proto_reflection_service: boolean

the other option could be complete separation. e.g. every config property is duplicated per service, so that they can be configured independently.

KarstenSchnitter · 2025-03-08T21:19:54Z

I like the configuration proposed by @TomasLongo. @dlvenable: What do you think about it?

KarstenSchnitter · 2025-04-09T15:11:14Z

@TomasLongo has rebased this PR onto main. It now contains authentication. @dlvenable and @kkondaka can you please give some feedback on the trace source changes. This PR is still a draft as this implementation is to be ported to metrics and logs as well.If you prefer smaller PRs, we can turn this PR from draft to ready for review and provide metrics and logs sources in different PRs. What is your take on this?

KarstenSchnitter · 2025-04-09T15:12:51Z

@TomasLongo please have a look at the failing end-to-end test. Please try to fix them. If the failures are not caused by this PR, please leave us a note about this as a comment.

TomasLongo · 2025-04-16T12:45:22Z

@KarstenSchnitter I could fix the checkstyle stuff. However, the test currently runs into a timeout, which I can not reproduce locally.

TomasLongo · 2025-05-14T14:12:29Z

@dlvenable @KarstenSchnitter I rebased the PR onto main. It's up to date now.

TomasLongo · 2025-05-16T10:23:12Z

All failed builds are docker builds. It seems like the download of the openjdk-17 is not possible. It quits with a 403.

KarstenSchnitter · 2025-05-19T14:40:56Z

Thanks for working on that change. I would recommend to mark this as ready for review. Based on the review of this PR, there will be more for the remaining OTel sources including the new one. @dlvenable, what is your thought on that?

…ccepts unframed requests Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Signed-off-by: Tomas Longo <tlongo@sternad.de>

KarstenSchnitter

LGTM. Changes to the other sources are going to be provided along the implementation in the OTelTraceSource.

Signed-off-by: Tomas Longo <tlongo@sternad.de>

KarstenSchnitter · 2025-10-27T08:39:56Z

...c/integrationTest/java/org/opensearch/dataprepper/integration/trace/EndToEndRawSpanTest.java

+        final ConnectionConfiguration.Builder builder = new ConnectionConfiguration.Builder(
+                Collections.singletonList("https://127.0.0.1:9200"));
+        builder.withUsername("admin");
+        builder.withPassword("admin");
+        builder.withInsecure(true);
+        final RestHighLevelClient restHighLevelClient = builder.build().createClient(null);
+        // Wait for data to flow through pipeline and be indexed by ES
+        await().atLeast(3, TimeUnit.SECONDS).atMost(20, TimeUnit.SECONDS).untilAsserted(
+                () -> {
+                    refreshIndices(restHighLevelClient);
+                    final SearchRequest searchRequest = new SearchRequest(INDEX_NAME);
+                    searchRequest.source(
+                            SearchSourceBuilder.searchSource()
+                                    .size(100)
+                                    .fetchField(TraceGroup.TRACE_GROUP_STATUS_CODE_FIELD)
+                                    .fetchField(TraceGroup.TRACE_GROUP_END_TIME_FIELD, "strict_date_time")
+                                    .fetchField(TraceGroup.TRACE_GROUP_DURATION_IN_NANOS_FIELD)
+                    );
+                    final SearchResponse searchResponse = restHighLevelClient.search(searchRequest, RequestOptions.DEFAULT);
+                    final List<Map<String, Object>> foundSources = getSourcesFromSearchHits(searchResponse.getHits());
+                    assertEquals(expectedDocuments.size(), foundSources.size());
+                    /**
+                     * Our raw trace prepper add more fields than the actual sent object. These are defaults from the proto.
+                     * So assertion is done if all the expected fields exists.
+                     *
+                     * TODO: Can we do better?
+                     *
+                     */
+                    expectedDocuments.forEach(expectedDoc -> {
+                        assertTrue(foundSources.stream()
+                                .filter(i -> i.get("spanId").equals(expectedDoc.get("spanId")))
+                                .findFirst().get()
+                                .entrySet().containsAll(expectedDoc.entrySet()));
+                    });
+                }
+        );


This block sits behind the return statement. It looks like a merge error. Furthermore it causes test failures, since expectedDocuments is undefined. @TomasLongo can you have a look at this?

KarstenSchnitter

There seems to have been a merge error. @TomasLongo can you have a look at this again?

Signed-off-by: Tomas Longo <tlongo@sternad.de>

KarstenSchnitter

Thanks for all the effort. I approve this PR. @dlvenable I am going to merge this if there are no further requests from your side.

kkondaka · 2026-02-06T00:01:40Z

@KarstenSchnitter @TomasLongo @dlvenable Looks like this broke the OTEL standard mode support because it ignore the output_format is ignored

KarstenSchnitter reviewed Jan 10, 2025

View reviewed changes

KarstenSchnitter reviewed Jan 21, 2025

View reviewed changes

KarstenSchnitter mentioned this pull request Jan 27, 2025

Upgrade OTEL version to 1.2 #5359

Closed

2 tasks

TomasLongo force-pushed the update-otel-proto branch from 6decceb to 090127c Compare February 13, 2025 07:47

TomasLongo mentioned this pull request Feb 13, 2025

Update otel proto buf specification #5434

Merged

4 tasks

TomasLongo force-pushed the update-otel-proto branch from 919f533 to 71eb4ab Compare March 7, 2025 14:08

TomasLongo force-pushed the update-otel-proto branch 2 times, most recently from 065b5b4 to bd62996 Compare April 16, 2025 07:12

KarstenSchnitter mentioned this pull request May 6, 2025

OTLP Source unified endpoint for logs, traces and metrics #5677

Merged

4 tasks

TomasLongo force-pushed the update-otel-proto branch from bd62996 to 84f74eb Compare May 14, 2025 13:44

TomasLongo marked this pull request as ready for review May 23, 2025 06:37

TomasLongo requested review from chenqi0805, engechas, san81 and sb2k16 as code owners May 23, 2025 06:37

Tomas Longo and others added 14 commits June 27, 2025 10:06

[WIP] Fix issue with http service being enabled, while grpc service a…

6c6fbff

…ccepts unframed requests Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Refactor EndToEndRawSpanTest

59003ab

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Create ArmeriaAuthenticationProvider via PluginFactory

f4c6fcf

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Rename GrpcRetryInfoCalculator

cf3b2e7

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Create test for invalid payload

c35d910

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Add test for healthcheck

19d2e99

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Add test for http exception handler

f56079f

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Remove tests

a342e14

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Fix missing imports

6c230f3

Signed-off-by: Tomas Longo <tlongo@sternad.de>

Remove unused imports

fa2e95d

Signed-off-by: Tomas Longo <tlongo@sternad.de>

Fix imports

4c694f5

Signed-off-by: Tomas Longo <tlongo@sternad.de>

Remove/edit todos

bfdc669

Signed-off-by: Tomas Longo <tlongo@sternad.de>

Remove accidentally added default password

702a568

Signed-off-by: Tomas Longo <tlongo@sternad.de>

Declare assertJ as test lib and reference it from e2e test

655abe4

Signed-off-by: Tomas Longo <tlongo@sternad.de>

TomasLongo force-pushed the update-otel-proto branch from 87630c2 to 655abe4 Compare June 27, 2025 08:07

KarstenSchnitter previously approved these changes Aug 18, 2025

View reviewed changes

KarstenSchnitter requested review from dlvenable and kkondaka August 26, 2025 07:42

Merge branch 'main' into update-otel-proto

23766cf

Signed-off-by: Tomas Longo <tlongo@sternad.de>

TomasLongo dismissed KarstenSchnitter’s stale review via 23766cf October 27, 2025 07:35

KarstenSchnitter reviewed Oct 27, 2025

View reviewed changes

KarstenSchnitter requested changes Oct 27, 2025

View reviewed changes

TomasLongo added 2 commits October 27, 2025 11:57

Fix merge error

fe1e86a

Signed-off-by: Tomas Longo <tlongo@sternad.de>

Use insecure ssl connection in test

39c8142

Signed-off-by: Tomas Longo <tlongo@sternad.de>

KarstenSchnitter approved these changes Oct 29, 2025

View reviewed changes

KarstenSchnitter merged commit bf397f3 into opensearch-project:main Oct 30, 2025
78 of 90 checks passed

KarstenSchnitter mentioned this pull request Jan 22, 2026

Expand OTLP Sink to support any compatible endpoint and support otlphttp protocol #6330

Open

dlvenable mentioned this pull request Feb 6, 2026

Fix OTEL trace source broken by PR 5322 #6494

Merged

2 tasks

dlvenable added this to the v2.13 milestone Feb 18, 2026

Conversation

TomasLongo commented Jan 10, 2025

Description

This draft PR serves as starting for further discussions

Configuration of HTTP and gRPC Service

Do we want to keep unframed requests

Usage of the plugin mechanism to handle configs

Issues Resolved

Check List

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

For Binary Protobufs

For JSON

Uh oh!

TomasLongo commented Jan 17, 2025

Uh oh!

KarstenSchnitter commented Jan 21, 2025

Uh oh!

KarstenSchnitter Jan 21, 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!

TomasLongo Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

dlvenable Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

TomasLongo Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

kkondaka commented Feb 3, 2025

Uh oh!

kkondaka commented Feb 7, 2025

Uh oh!

TomasLongo commented Feb 13, 2025

Uh oh!

TomasLongo commented Feb 14, 2025

Uh oh!

TomasLongo commented Feb 21, 2025

Uh oh!

KarstenSchnitter commented Mar 8, 2025

Uh oh!

KarstenSchnitter commented Apr 9, 2025

Uh oh!

KarstenSchnitter commented Apr 9, 2025

Uh oh!

TomasLongo commented Apr 16, 2025

Uh oh!

TomasLongo commented May 14, 2025

Uh oh!

TomasLongo commented May 16, 2025

Uh oh!

KarstenSchnitter commented May 19, 2025

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TomasLongo commented Jan 17, 2025 •

edited

Loading

KarstenSchnitter Jan 21, 2025 •

edited

Loading