Refactor otlp exporter #146
Conversation
| "otlp_http": { | ||
| "$ref": "common.json#/$defs/OtlpHttpExporter" | ||
| }, | ||
| "otlp_grpc": { |
There was a problem hiding this comment.
There's a question of whether we should align with the names of exporters in the collector, which follows the same strategy of splitting the exporter by transport:
I've chosen to not align with the collector for a couple of reasons:
- The collector follows a different case naming convention than this project. We prefer lower snake case, while the collector prefers something that I've seen called flatcase or mumblecase. We'd have to break our case policy to align with the collector.
- The collector prefers gRPC, which is an out of date default. By omitting a suffix on the grpc variant (i.e.
otlpvsotlpgrpcandotlphttpinstead ofotlp), the collector is expressing a subtle preference for grpc. This was the original default for OTLP but OTEL has since switched course preferring HTTP. In the naming convention I've chosen, HTTP and gRPC are presented as equals. I don't think we should express a preference, but if we did, it should be for HTTP (i.e.otlpfor HTTP andotlpgrpcfor gRPC). This would be confusing for collector users, so better to explicitly break rank.
There was a problem hiding this comment.
I know we're still not providing backwards compatibility guarantees, but changes like this (and previously the headers change) should be captured somewhere as being breaking changes.
Maybe it's easy enough to label the title of this PR breaking: refactor OTLP exporter so that at release time these are listed in the release notes
Let's do this. We can add a note to the release workflow indicating that breaking changes should be called out clearly in the changelog. |
…y-configuration into refactor-otlp-exporter
|
Wondering if any other @open-telemetry/configuration-approvers care about this / plan on reviewing it. Happy to leave it open longer if more time is needed. |
I'd like a day or two to review this please |
|
At this point, might as well leave it open until the new year. No rush as this doesn't hold up anything between now and then. |
|
Here's an alternative for consideration. I also included the in-development otlp file exporter config.yamltracer_provider:
processors:
- batch:
exporter:
otlp:
transport:
http:
encoding: protobuf
headers:
- name: api-key
value: "1234"
endpoint: http://localhost:4318/v1/traces
timeout: 10000
- batch:
exporter:
otlp:
transport:
http:
encoding: json
endpoint: http://localhost:4318/v1/traces
compression: gzip
timeout: 10000
- batch:
exporter:
otlp:
transport:
grpc:
insecure: true
endpoint: localhost:4317
certificate: /app/cert.pem
client_certificate: /app/cert.pem
- batch:
exporter:
otlp:
transport:
file:
stream: stdout #default value
- batch:
exporter:
otlp:
transport:
file:
stream: /path/to/traces.jsonlThe |
Resolves #103.
otlpexporter intootlp_httpfor HTTP transport, andotlp_grpcfor gRPC transportotlp.protocolpropertyotlp.endpointproperty from required, since there is now a reasonable default.otlp_httpandotlp_grpccan both be set to null. Since there are no longer any required properties, its reasonable for a user to specifyotlp_http:,otlp_grpc:..insecureproperty is only applicable tootlp_grpcencodingproperty with valuesprotoorjsonforotlp_http. Default isproto.Unrelated changes bundled in:
*ExporterprefixI've prototyped this change in opentelemetry-java here.