-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce otlp/http support #5322
base: main
Are you sure you want to change the base?
Introduce otlp/http support #5322
Conversation
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
…quests Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
Signed-off-by: Tomas Longo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
ResourceSpans resourceSpans = ResourceSpans.newBuilder() | ||
.setResource(resource) | ||
.addScopeSpans(scopeSpans) | ||
.addInstrumentationLibrarySpans(ilSpans) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, that instrumentation library was migrated to instrumentation scope. Is this removal required to address the changed data model?
@@ -71,60 +69,6 @@ void init() { | |||
rawProcessor = new OTelMetricsRawProcessor(testsettings, new OtelMetricsRawProcessorConfig()); | |||
} | |||
|
|||
@Test | |||
void testInstrumentationLibrary() throws JsonProcessingException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this test still be required for backward compatibility?
@@ -350,7 +351,7 @@ void testHttpFullJsonWithCustomPathAndUnframedRequests() throws InvalidProtocolB | |||
.join(); | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check you IDE settings for the correct formatter. There should be no whitespace changes.
|
||
|
||
final ResourceMetrics resourceMetrics = ResourceMetrics.newBuilder() | ||
.setResource(resource) | ||
.addInstrumentationLibraryMetrics(isntLib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially, this is now an empty resource metric, right?
Stream<OpenTelemetryLog> mappedInstrumentationLibraryLogs = rs.getInstrumentationLibraryLogsList() | ||
.stream() | ||
.map(ils -> | ||
processLogsList(ils.getLogRecordsList(), | ||
serviceName, | ||
OTelProtoCodec.getInstrumentationLibraryAttributes(ils.getInstrumentationLibrary()), | ||
resourceAttributes, | ||
schemaUrl, | ||
timeReceived)) | ||
.flatMap(Collection::stream); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this break backward compatibility with old OTel data models?
Description
Introduce http endpoint to support otlp/http for the otel trace source. So far, this draft PR contains:
/opentelemetry.proto.collector.trace.v1.TraceService/Export
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:
unframed_requests
,proto_reflection_service
)compression
,authentication
) and should/couldThis leads to the questions how the structure of the config should look like in the future
Create distinct sections for every endpoint
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
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.