feat: add OTLP/JSON (http/json) export support#2238
Conversation
The protocol defaults to http/protobuf, but can be set to http/json through the respective environment variables. Includes minor format adjustments to satisfy Rubocop rules
…liance - enums must be emitted as integers - Protobuf encodes strings as base64, but trace_ids and span_ids are hex Includes some minor changes to satisfy Rubocop
| @uri = if endpoint == ENV['OTEL_EXPORTER_OTLP_ENDPOINT'] | ||
| endpoint += '/' unless endpoint.end_with?('/') | ||
| URI.join(endpoint, 'v1/logs') | ||
| else | ||
| URI(endpoint) | ||
| end | ||
|
|
||
| @uri = prepare_endpoint(endpoint) |
There was a problem hiding this comment.
Let's put this back. Note i have targeted pr to centralise the logic in common.
| # As JSON etsr (ExportTraceServiceRequest) | ||
| # | ||
| # Serializes to spec-compliant OTLP/JSON: trace/span ids as hex strings and | ||
| # enum values as integers. | ||
| # | ||
| # @param [Enumerable<OpenTelemetry::SDK::Trace::SpanData>] span_data the | ||
| # list of recorded {OpenTelemetry::SDK::Trace::SpanData} structs to be | ||
| # encoded. | ||
| # | ||
| # @return [String] returns a JSON encoded ETSR of the provided span data | ||
| def as_json_etsr(span_data) | ||
| as_etsr(span_data, json: true).to_json(format_enums_as_integers: true) | ||
| rescue StandardError => e | ||
| OpenTelemetry.handle_error(exception: e, message: 'unexpected error in OTLP::Common#as_json_etsr') | ||
| nil | ||
| end | ||
|
|
There was a problem hiding this comment.
Can we consolidate this into the existing method with a new protocol parameter to control the difference.
| send_bytes(OpenTelemetry::Exporter::OTLP::Common.as_encoded_etsr(span_data), timeout: timeout) | ||
| bytes = if @content_type == 'application/json' | ||
| OpenTelemetry::Exporter::OTLP::Common.as_json_etsr(span_data) | ||
| else | ||
| OpenTelemetry::Exporter::OTLP::Common.as_encoded_etsr(span_data) | ||
| end | ||
| send_bytes(bytes, timeout: timeout) |
There was a problem hiding this comment.
Update based on above
| if @content_type == 'application/json' | ||
| as_export_logs_service_request(log_record_data, json: true).to_json(format_enums_as_integers: true) | ||
| else | ||
| Opentelemetry::Proto::Collector::Logs::V1::ExportLogsServiceRequest.encode(as_export_logs_service_request(log_record_data)) | ||
| end |
There was a problem hiding this comment.
| if @content_type == 'application/json' | |
| as_export_logs_service_request(log_record_data, json: true).to_json(format_enums_as_integers: true) | |
| else | |
| Opentelemetry::Proto::Collector::Logs::V1::ExportLogsServiceRequest.encode(as_export_logs_service_request(log_record_data)) | |
| end | |
| json = @content_type == 'application/json' | |
| payload = as_export_logs_service_request(log_record_data, json: json) | |
| if json | |
| payload.to_json(format_enums_as_integers: true) | |
| else | |
| Opentelemetry::Proto::Collector::Logs::V1::ExportLogsServiceRequest.encode(payload) | |
| end |
| @uri = if endpoint == ENV['OTEL_EXPORTER_OTLP_ENDPOINT'] | ||
| endpoint += '/' unless endpoint.end_with?('/') | ||
| URI.join(endpoint, 'v1/metrics') | ||
| else | ||
| URI(endpoint) | ||
| end | ||
|
|
||
| @uri = prepare_endpoint(endpoint) |
| if @content_type == 'application/json' | ||
| as_export_metrics_service_request(metrics_data, json: true).to_json(format_enums_as_integers: true) | ||
| else | ||
| Opentelemetry::Proto::Collector::Metrics::V1::ExportMetricsServiceRequest.encode(as_export_metrics_service_request(metrics_data)) | ||
| end |
There was a problem hiding this comment.
Repeat previous pattern
| def prepare_endpoint(endpoint) | ||
| return URI(endpoint) unless endpoint == ENV['OTEL_EXPORTER_OTLP_ENDPOINT'] | ||
|
|
||
| endpoint += '/' unless endpoint.end_with?('/') | ||
| URI.join(endpoint, 'v1/metrics') | ||
| end |
| # @return [Opentelemetry::Proto::Collector::Trace::V1::ExportTraceServiceRequest] | ||
| # returns an ETSR of the provided span data | ||
| def as_etsr(span_data) | ||
| def as_etsr(span_data, json: false) |
There was a problem hiding this comment.
Would it be better to avoid a mixture of named & positional arguments so something like
| def as_etsr(span_data, json: false) | |
| def as_etsr(span_data, json = false) |
There was a problem hiding this comment.
I really like your idea of using a positional argument here.
What do you think about the name format? (either :json or :protobuf); that term keeps the focus squarely on serialization and avoids any confusion with other terminology.
Also, what are your thoughts on making this a named argument for public methods, but keep it positional for private ones?
…protocol. - common.rb now accepts a `format` parameter that is either :json or :protobuf. - logs_exporter + metrics_exporter pass json as positional argument when converting to request. - trace_exporter derives format from the protocol. Thank you the feedback thompson-tomo open-telemetry#1874
What type of PR is this?
/kind feature
What this PR does / why we need it:
implements the http/json export capability.
Which issue(s) this PR fixes:
Addresses #1874
Special notes for your reviewer:
The enums are exported as integers and the ids retain their hex encoding, as per the specification.
For retaining the hex values, I took inspiration from the PHP implementation, which also base64 decodes the ids before passing marshaling.
For the enum values, I bumped google-protobuf to 3.23.0. I felt this was the easiest way of meeting the specification without building a custom workaround.
There are some minor style changes to satisfy Rubocop.
Does this PR introduce a user-facing change?
Additional documentation: