Fix OTEL trace source broken by PR 5322#6494
Fix OTEL trace source broken by PR 5322#6494kkondaka merged 3 commits intoopensearch-project:mainfrom
Conversation
Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>
KarstenSchnitter
left a comment
There was a problem hiding this comment.
Thanks for fixing this, I think, you missed the ArmeriaHttpService: https://github.com/kkondaka/kk-data-prepper-f2/blob/fb32017455fb5092ecf9cae4d938c7589617624b/data-prepper-plugins/otel-trace-source/src/main/java/org/opensearch/dataprepper/plugins/source/oteltrace/http/ArmeriaHttpService.java#L55
Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>
| } | ||
|
|
||
| @Test | ||
| void testCreateWithAuthentication() { |
There was a problem hiding this comment.
Is there a reason to assert internals instead of treating the service as black box and assert its behaviour?
This is a good example for it. The test could bootstrap the trace source and make sure that it doesn't accept requests that do not carry authentication information.
In this form, the test depends on the exact invocation of some internal component.
I'm addressing this here, because in #6250 I followed my line of reasoning getting rid of all the mocking, bootstrapping the service and testing with actual requests against it. Is that something to avoid?
There was a problem hiding this comment.
I think as a unit test, it is appropriate to have something like this. Though, we should also have tests that are more black box. Using a real server and web client provide much better tests for authentication.
dlvenable
left a comment
There was a problem hiding this comment.
I left some comments. I also see a number of failing GitHub Actions.
|
|
||
| ExportTraceServiceResponse response = armeriaHttpService.exportTrace(request); | ||
|
|
||
| assertNotNull(response); |
There was a problem hiding this comment.
Let's move as much out of the try-with-resources block as possible. I'm fairly sure that the rest of this can go out of it.
| verify(successRequestsCounter).increment(); | ||
| assertThat(recordsReceived.size(), equalTo(1)); | ||
| Span span = recordsReceived.get(0).getData(); | ||
| if (outputFormat == OTelOutputFormat.OTEL) { |
There was a problem hiding this comment.
Let's avoid conditions. Instead, make an ArgumentsProvider class which produces these as inputs.
Something like this:
static class OTelOutputFormatToServiceNameKey implements ArgumentsProvider {
@Override
public Stream<? extends Arguments> provideArguments(final ExtensionContext extensionContext) {
return Stream.of(
arguments(OTelOutputFormat.OPENSEARCH, "attributes/resource.attributes.service@name"),
arguments(OTelOutputFormat.OTEL, "resource/attributes/service.name")
);
}
}
Then you just have:
assertThat(span.get(serviceNameKey, String.class), equalTo(TRACE_SERVICE_NAME));
| } | ||
|
|
||
| @Test | ||
| void testCreateWithAuthentication() { |
There was a problem hiding this comment.
I think as a unit test, it is appropriate to have something like this. Though, we should also have tests that are more black box. Using a real server and web client provide much better tests for authentication.
| public ArmeriaHttpService(Buffer<Record<Object>> buffer, final OTelOutputFormat otelOutputFormat, final PluginMetrics pluginMetrics, final int bufferWriteTimeoutInMillis) { | ||
| this.buffer = buffer; | ||
| this.oTelProtoDecoder = new OTelProtoOpensearchCodec.OTelProtoDecoder(); | ||
| this.oTelProtoDecoder = (otelOutputFormat == OTelOutputFormat.OPENSEARCH) ? new OTelProtoOpensearchCodec.OTelProtoDecoder() : new OTelProtoStandardCodec.OTelProtoDecoder(); |
There was a problem hiding this comment.
This is the same as the change above. We'd be better off injecting the OTelProtoDecoder into these classes and making this decision in a single location.
|
Actually I wanted @TomasLongo to write the unit tests separately because they were not there when the PR was merged. But I quickly generated few unit tests using AI :-) |
Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>
dlvenable
left a comment
There was a problem hiding this comment.
Thanks @kkondaka .
@KarstenSchnitter , Can you review again?
|
@KarstenSchnitter please review this. |
KarstenSchnitter
left a comment
There was a problem hiding this comment.
Thanks for providing the fixe and adding the tests.
Description
Fix OTEL trace source broken by PR #5322
The PR was started before OTEL standard support was added committed long after OTEL standard support was added. This resulted in regression of support for OTEL standard in otel-trace-source
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
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.