Skip to content
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

OTLP exporter #277

Merged
merged 13 commits into from
Sep 9, 2020
Merged

OTLP exporter #277

merged 13 commits into from
Sep 9, 2020

Conversation

fbogsany
Copy link
Contributor

@fbogsany fbogsany commented Jun 13, 2020

This adds a gem containing an exporter for OTLP over HTTP (trace only).

To do:

  • Generate protos
  • Distinguish failure modes and retry with exponential back off
  • Translate Status
  • Avoid tracing the export request (needs SDK support)
  • GZIP (Requires upstream support)
  • Unit tests
  • Manual (and perhaps automated?) integration test against the collector
  • Array-valued attributes
  • Handling redirects (Punting to a subsequent PR)
  • Rubocop warnings (somehow not generated by CI 🤔 )
  • user specified headers
  • group by span.resource
  • environment variable config
  • rename the gem to opentelemetry-exporter-otlp-http (gRPC support will be added later as opentelemetry-exporter-otlp-grpc)

Blockers

@fbogsany fbogsany self-assigned this Jun 13, 2020
@fbogsany fbogsany linked an issue Jun 13, 2020 that may be closed by this pull request
@fbogsany fbogsany marked this pull request as ready for review July 30, 2020 02:13
@fbogsany
Copy link
Contributor Author

There are two open issues:

  1. Array-valued attributes
  2. Handling redirects

Otherwise this is ready for review.

@fbogsany
Copy link
Contributor Author

@ericmustin I'd appreciate your 👀 on this, since you have some experience implementing exporters. 🙏

end

it 'integrates with collector' do
skip unless ENV['TRACING_INTEGRATION_TEST']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run this locally with:

TRACING_INTEGRATION_TEST=1 bundle exec rake test

and the collector:

./otelcol --config logging.yml

where logging.yml is:

receivers:
  otlp:
    protocols:
      http:
exporters:
  logging:
extensions:
  zpages:
service:
  extensions: [zpages]
  pipelines:
    traces:
      receivers: [otlp]
      exporters: [logging]

It produces the output:

2020-07-29T22:05:48.751-0400	INFO	loggingexporter/logging_exporter.go:102	TraceExporter	{"#spans": 1}

And metrics:

Franciss-MacBook-Pro:otlp bogsanyi$ curl http://localhost:8888/metrics
# HELP otelcol_exporter_send_failed_spans Number of spans in failed attempts to send to destination.
# TYPE otelcol_exporter_send_failed_spans counter
otelcol_exporter_send_failed_spans{exporter="logging",service_instance_id="d5ec4873-08ce-4142-b8ea-c282714a95be"} 0
# HELP otelcol_exporter_sent_spans Number of spans successfully sent to destination.
# TYPE otelcol_exporter_sent_spans counter
otelcol_exporter_sent_spans{exporter="logging",service_instance_id="d5ec4873-08ce-4142-b8ea-c282714a95be"} 1
# HELP otelcol_process_runtime_heap_alloc_bytes Bytes of allocated heap objects (see 'go doc runtime.MemStats.HeapAlloc')
# TYPE otelcol_process_runtime_heap_alloc_bytes gauge
otelcol_process_runtime_heap_alloc_bytes{service_instance_id="d5ec4873-08ce-4142-b8ea-c282714a95be"} 4.49036e+06
# HELP otelcol_process_runtime_total_alloc_bytes Cumulative bytes allocated for heap objects (see 'go doc runtime.MemStats.TotalAlloc')
# TYPE otelcol_process_runtime_total_alloc_bytes gauge
otelcol_process_runtime_total_alloc_bytes{service_instance_id="d5ec4873-08ce-4142-b8ea-c282714a95be"} 9.477112e+06
# HELP otelcol_process_runtime_total_sys_memory_bytes Total bytes of memory obtained from the OS (see 'go doc runtime.MemStats.Sys')
# TYPE otelcol_process_runtime_total_sys_memory_bytes gauge
otelcol_process_runtime_total_sys_memory_bytes{service_instance_id="d5ec4873-08ce-4142-b8ea-c282714a95be"} 7.2286456e+07
# HELP otelcol_receiver_accepted_spans Number of spans successfully pushed into the pipeline.
# TYPE otelcol_receiver_accepted_spans counter
otelcol_receiver_accepted_spans{receiver="otlp",service_instance_id="d5ec4873-08ce-4142-b8ea-c282714a95be",transport="grpc"} 1
# HELP otelcol_receiver_refused_spans Number of spans that could not be pushed into the pipeline.
# TYPE otelcol_receiver_refused_spans counter
otelcol_receiver_refused_spans{receiver="otlp",service_instance_id="d5ec4873-08ce-4142-b8ea-c282714a95be",transport="grpc"} 0

@ericmustin
Copy link
Contributor

@fbogsany yup happy to look, should be able to block off some time in the coming days...one thing that jumps out is it isn't immediately clear to me how Avoid tracing the export request (needs SDK support) is getting accomplished? Is there a separate PR here or something implicit in the Net::HTTP instrumentation?

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some small comments but there's nothing blocking for me. Not that my approval is needed here, but as there are still a few TODOs you have mentioned I've held off on giving an approval until those are implemented, but overall this LGTM and am happy to approve if you want to kick the TODOs to a later PR. Nice work on this!

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, LGTM. I have couple of comments though.

@fbogsany
Copy link
Contributor Author

fbogsany commented Sep 3, 2020

I've rebased this and made some minor fixes for API changes. I haven't addressed feedback yet, or the open issues.

@robertlaurin
Copy link
Contributor

Rubocop warnings (somehow not generated by CI 🤔 )

https://github.com/robertlaurin/opentelemetry-ruby/blob/2913c9e13a12e7d73e864c85a5806ddfc38f3ff9/.circleci/config.yml#L61-L69

bundle exec rake will run all the rake tasks (including rubocop) and I think OTLP is missing from CI all together right now.

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few small questions about making sure we safely encode spans that may not have resource and instrumentation_library, and left suggestions about the custom header support, but i'm fine with any approach here, and if we can't reach consensus i'm also fine pushing this feature to a later PR/release. There also look to be one or two ENV Var tests that are still marked as TODOs, if you want to punt on those to a later PR i think that's completely fine as well.

I think this is very close imo, the only real blocker to me is that jruby appears to be having some dependancy issues with the added gems.

@fbogsany
Copy link
Contributor Author

fbogsany commented Sep 9, 2020

I think I've done everything I said I would, except for:

rename the gem to opentelemetry-exporter-otlp-http

I honestly lack the energy to do this. I'd rather merge what we have here and take some time afterwards to talk through how to handle the OTEL_EXPORTER_OTLP_PROTOCOL config option. I'm less convinced now that the gem rename is the right approach (but I also don't know what the "right thing" is).

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather merge what we have here and take some time afterwards to talk through how to handle the OTEL_EXPORTER_OTLP_PROTOCOL config option. I'm less convinced now that the gem rename is the right approach (but I also don't know what the "right thing" is).

Concur, let's not let perfect be the enemy of good as this is very robust.

@fbogsany fbogsany merged commit 669f794 into master Sep 9, 2020
@fbogsany fbogsany deleted the otlp-exporter branch September 9, 2020 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exporters: implement OpenTelemetry protocol exporter
6 participants